refactor: change extra_field from Arc<Vec<u8>> to Arc<[u8]>#741
Conversation
Problem: ZipFileData stores extra_field and central_extra_field as
Option<Arc<Vec<u8>>>. This requires two pointer dereferences to
reach the data (Arc ptr → Vec ptr → heap buffer), and the Vec's
3-word header (ptr, len, capacity) wastes space since the data is
never resized after construction.
Fix: change to Option<Arc<[u8]>>, which stores the byte slice
inline in the Arc allocation. One fewer indirection, and 16 bytes
saved per field (no Vec header or capacity).
The AES extra data mutation path (update_aes_extra_data) can no
longer use Arc::make_mut since [u8] is unsized. Instead it copies
to a Vec, mutates, and converts back — acceptable since it only
runs once during AES encryption setup.
Measured on bazel.jar (23,631 Stored entries):
ZipArchive::new() net memory per entry:
before: 490 bytes/entry
after: 482 bytes/entry (-1.6%)
(allocation count unchanged at 4.0/entry — this change reduces
per-allocation overhead, not allocation count)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes the storage of extra data fields within Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
|
/gemini review |
Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request optimizes memory usage in the ZipFileData struct by replacing Arc<Vec<u8>> with Arc<[u8]> for extra fields, thereby eliminating a level of pointer indirection. The implementation involves updating several files to use Arc::from and boxed slices during field initialization and modification. I have no feedback to provide.
|
I thought this would be a MAJOR breaking |
|
Maybe there's a special exception for switching between Vec and slice, just because Vec is so rarely used otherwise than through |
ZipFileData currently stores extra_field and central_extra_field as
Option<Arc<Vec<u8>>>. This requires two pointer dereferences to reach the data (Arc ptr → Vec ptr → heap buffer), and the Vec's 3-word header (ptr, len, capacity) wastes space since the data is never resized after construction.This changes the fields to Option<Arc<[u8]>>, which stores the byte slice inline in the Arc allocation. One indirection instead of two, and one usize saved per field (no capacity).
The AES extra data mutation path (update_aes_extra_data) can no longer use Arc::make_mut since [u8] is unsized. Instead it copies to a Vec, mutates, and converts back. I think it's acceptable since it only runs once during AES encryption setup on the write path.
This is a breaking change since the fields are exposed publicly