Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New direct seekTable access methods #2518

Merged
merged 13 commits into from
Mar 4, 2021
Merged

New direct seekTable access methods #2518

merged 13 commits into from
Mar 4, 2021

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Mar 3, 2021

This is a follow up of #2113, incorporating several comments made on this PR,
plus a few minor refactoring operations, both for style and more robust compilation.

Additional topics to consider :

  • The reasoning for the creation of this new abstraction layer should be better documented (done)
  • There should be at least one automated test involving the new abstraction (done)
  • I'm puzzled about the object's name (ZSTD_seekTable) which feels a bit too close to existing ZSTD_seekable. Basically, one letter difference. The type system should avoid mixing wrong types, but it's still a potential source of confusion. Problem is, I haven't found any better name (no change)
  • Not directly related, but I noticed that I/O FILE* functions are present in the same unit as memory-only ones. Seems like they'd better be separate, as it's entirely possible for an application to only need memory access functions, without the need to drag <stdio.h> dependencies (separate PR)

This PR most likely conflicts with #2516 , though hopefully nothing too difficult to fix. (edit: #2516 is now merged into this PR)

mdittmer and others added 6 commits May 7, 2020 09:31
Memory constrained use cases that manage multiple archives benefit from
retaining multiple archive seek tables without retaining a ZSTD_seekable
instance for each.

* New opaque type for seek table: ZSTD_seekTable.
* ZSTD_seekable_copySeekTable() supports copying seek table out of a
  ZSTD_seekable.
* ZSTD_seekTable_[eachSeekTableOp]() defines seek table API that mirrors
  existing seek table operations.
* Existing ZSTD_seekable_[eachSeekTableOp]() retained; they delegate to
  ZSTD_seekTable the variant.

These changes allow the above-mentioned use cases to initialize a
ZSTD_seekable, extract its ZSTD_seekTable, then throw the ZSTD_seekable
away to save memory. Standard ZSTD operations can then be used to
decompress frames based on seek table offsets.

The copy and delegate patterns are intended to minimize impact on
existing code and clients. Using copy instead of move for the infrequent
operation extracting a seek table ensures that the extraction does not
render the ZSTD_seekable useless. Delegating to *new* seek
table-oriented APIs ensures that this is not a breaking change for
existing clients while supporting all meaningful operations that depend
only on seek table data.
[contrib] Support seek table-only API
read-only objects are properly const-ified in parameters
Copy link
Contributor

@senhuang42 senhuang42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me! The roundtrip unit test is a great addition.

@Cyan4973 Cyan4973 merged commit c4d54ab into dev Mar 4, 2021
@Cyan4973 Cyan4973 deleted the seekTable branch May 4, 2021 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants