Skip to content

read the zlib dictionary length once in CopyDictionary - #63859

Open
ubeddulla wants to merge 2 commits into
dart-lang:mainfrom
ubeddulla:zlib-dictionary-length
Open

read the zlib dictionary length once in CopyDictionary#63859
ubeddulla wants to merge 2 commits into
dart-lang:mainfrom
ubeddulla:zlib-dictionary-length

Conversation

@ubeddulla

Copy link
Copy Markdown
Contributor

CopyDictionary in runtime/bin/filter.cc sizes the dictionary heap buffer from one Dart_ListLength call, and Filter_CreateZLibInflate / Filter_CreateZLibDeflate then call Dart_ListLength on the same object a second time to get the length they hand to the filter. For a user-defined class implementing List<int> that call dispatches to the Dart length getter, so the two reads can disagree, and a getter that returns a bigger value the second time leaves dictionary_length_ pointing past the end of the allocation that inflateSetDictionary / deflateSetDictionary later read from. ZLibCodec, ZLibEncoder and ZLibDecoder all pass the caller's List<int> straight through to these natives, so no copy happens on the Dart side first.

Having CopyDictionary report the number of bytes it actually copied keeps the buffer and the length in sync and drops the second lookup at both call sites.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:
  • See our contributor guide for general expectations for PRs.
  • Larger or significant changes should be discussed in an issue before creating a PR.
  • Contributions to our repos should follow the Dart style guide and use dart format.

Note that this repository uses Gerrit for code reviews. Your pull request will be automatically converted into a Gerrit CL and a link to the CL written into this PR. The review will happen on Gerrit but you can also push additional commits to this PR to update the code review.

@copybara-service

Copy link
Copy Markdown

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/526160

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

@ubeddulla

Copy link
Copy Markdown
Contributor Author

any update?

@copybara-service

Copy link
Copy Markdown

CL has new comments, please view and respond to them in Gerrit.

If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews.

Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send.

Introduce Filter::Dictionary, a std::unique_ptr<uint8_t[]> paired with the
length that was actually copied, and pass it around by move instead of a raw
pointer plus a separate length. CopyDictionary now drops the manual
Dart_TypedDataAcquireData fast path and just uses Dart_ListGetAsBytes, which
already handles typed data internally.

Per review comments on the CL.
@ubeddulla

Copy link
Copy Markdown
Contributor Author

Pushed an update for both comments.

The dictionary is now a Filter::Dictionary struct (a std::unique_ptr<uint8_t[]> plus its length) that gets moved into the filter, so the buffer and length are always in sync and the manual delete[] calls are gone. I put the struct on the base Filter so both the inflate and deflate filters can share it.

Also dropped the hand-rolled Dart_TypedDataAcquireData fast path in CopyDictionary and just call Dart_ListGetAsBytes, since it already handles the typed-data case internally.

@copybara-service

Copy link
Copy Markdown

https://dart-review.googlesource.com/c/sdk/+/526160 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service

Copy link
Copy Markdown

https://dart-review.googlesource.com/c/sdk/+/526160 has been updated with the latest commits from this pull request.

@copybara-service

Copy link
Copy Markdown

CL has new comments, please view and respond to them in Gerrit.

If a reviewer requested changes, push new commits to this PR and it will be automatically copied to Gerrit. After that you can mark reviewer comments as resolved in Gerrit and request another round of reviews.

Note: when you add comments in Gerrit they only become visible after you send them by clicking Reply and Send.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant