Skip to content

refactor: remove useless magic in struct#730

Open
Its-Just-Nans wants to merge 58 commits intomasterfrom
remove-useless-magic
Open

refactor: remove useless magic in struct#730
Its-Just-Nans wants to merge 58 commits intomasterfrom
remove-useless-magic

Conversation

@Its-Just-Nans
Copy link
Member

  • tiny struct

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, 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 introduces a significant refactoring of how fixed-size blocks, particularly those with magic signatures, are handled within the codebase. The primary goal is to decouple the magic bytes from the core data structures, improving modularity and clarity. This change impacts how data blocks are parsed from and written to streams, and ensures that offset calculations correctly reflect the new structure.

Highlights

  • Refactored FixedSizeBlock Trait: The FixedSizeBlock trait has been refactored to separate the magic signature bytes from the data structure itself. A new BlockWithMagic wrapper struct now handles the reading and writing of the magic bytes, simplifying the FixedSizeBlock implementations.
  • Improved Magic Byte Handling: Reading and writing operations for blocks that include a magic signature now explicitly handle the magic bytes separately using the new BlockWithMagic struct, leading to clearer code and more robust validation.
  • Updated Offset Calculations: Various offset calculations throughout the codebase, particularly in read.rs, write.rs, and types.rs, have been updated to correctly account for the separate handling of magic bytes, ensuring accurate file parsing and writing.
  • Stream Reading Enhancements: The stream reading functions in read.rs and read/stream.rs have been updated to first read only the magic bytes for identification before attempting to read the full block, and a new test for compressed stream reading was added.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@amazon-q-developer amazon-q-developer bot left a comment

Choose a reason for hiding this comment

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

This PR refactors the ZIP specification handling by separating magic numbers from data blocks in a BlockWithMagic wrapper structure. The changes are well-structured and improve code organization by making magic numbers explicit in the serialization/deserialization process.

One critical issue found that must be fixed before merging: a debug print statement (eprintln!) was left in production code in src/spec.rs at line 264.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request is a nice refactoring that removes the explicit magic field from several structs and centralizes the magic number handling within the FixedSizeBlock trait by introducing a BlockWithMagic wrapper. This simplifies the struct definitions and makes the code cleaner. The changes are consistent across the codebase. I've found one minor issue with a leftover debug print statement that should be removed.

src/spec.rs Outdated
Comment on lines +263 to +264
let BlockWithMagic { inner, .. } = block;
eprintln!("{} {}", block.as_bytes().len(), inner.as_bytes().len());
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This eprintln! and the unused BlockWithMagic destructuring appear to be leftover from debugging. They should be removed to avoid printing to stderr during normal operation.

@Its-Just-Nans Its-Just-Nans changed the title refactor: remove useless magic in struct DRAFT: refactor: remove useless magic in struct Mar 18, 2026
@Its-Just-Nans Its-Just-Nans marked this pull request as draft March 18, 2026 05:52
@Its-Just-Nans Its-Just-Nans changed the base branch from master to Its-Just-Nans-patch-1 March 19, 2026 00:20
@Its-Just-Nans Its-Just-Nans changed the base branch from Its-Just-Nans-patch-1 to master March 19, 2026 00:21
@Its-Just-Nans Its-Just-Nans self-assigned this Mar 19, 2026
@Its-Just-Nans Its-Just-Nans marked this pull request as ready for review March 19, 2026 15:11
@Its-Just-Nans
Copy link
Member Author

I don't really see any worst perf. Benching is hard, and I easily get good result and worst

On this branch

test read_many_entries             ... bench:  10,858,598 ns/iter (+/- 136,358) = 9656 MB/s
test read_many_entries_as_file     ... bench:  17,907,072 ns/iter (+/- 100,973) = 5855 MB/s
test read_many_entries_as_file_buf ... bench:  17,819,705 ns/iter (+/- 189,529) = 5884 MB/s

test read_many_entries             ... bench:  10,717,593 ns/iter (+/- 172,042) = 9783 MB/s
test read_many_entries_as_file     ... bench:  18,060,808 ns/iter (+/- 208,258) = 5805 MB/s
test read_many_entries_as_file_buf ... bench:  17,983,852 ns/iter (+/- 206,274) = 5830 MB/s

test read_many_entries             ... bench:  10,739,503 ns/iter (+/- 166,921) = 9763 MB/s
test read_many_entries_as_file     ... bench:  18,035,231 ns/iter (+/- 220,847) = 5814 MB/s
test read_many_entries_as_file_buf ... bench:  17,978,481 ns/iter (+/- 143,873) = 5832 MB/s

On the master branch

test read_many_entries             ... bench:  10,873,202 ns/iter (+/- 133,429) = 9643 MB/s
test read_many_entries_as_file     ... bench:  17,769,760 ns/iter (+/- 177,317) = 5900 MB/s
test read_many_entries_as_file_buf ... bench:  17,764,071 ns/iter (+/- 197,462) = 5902 MB/s

test read_many_entries             ... bench:  10,764,432 ns/iter (+/- 132,070) = 9741 MB/s
test read_many_entries_as_file     ... bench:  18,020,543 ns/iter (+/- 149,855) = 5818 MB/s
test read_many_entries_as_file_buf ... bench:  17,975,996 ns/iter (+/- 138,527) = 5833 MB/s

test read_many_entries             ... bench:  10,708,904 ns/iter (+/- 127,232) = 9791 MB/s
test read_many_entries_as_file     ... bench:  17,879,023 ns/iter (+/- 128,727) = 5864 MB/s
test read_many_entries_as_file_buf ... bench:  17,808,264 ns/iter (+/- 116,459) = 5888 MB/s

Seems to be silghty faster on single read entry but still the same, always different results

On this branch

test read_entry ... bench:     100,038 ns/iter (+/- 1,892) = 10481 MB/s
test read_entry ... bench:      99,378 ns/iter (+/- 3,091) = 10551 MB/s

On master

test read_entry ... bench:     100,080 ns/iter (+/- 4,325) = 10477 MB/s
test read_entry ... bench:     100,529 ns/iter (+/- 4,313) = 10430 MB/s
test read_entry ... bench:      99,491 ns/iter (+/- 6,131) = 10539 MB/s

So this MR is more for code refactoring purpose

@Its-Just-Nans Its-Just-Nans changed the title DRAFT: refactor: remove useless magic in struct refactor: remove useless magic in struct Mar 21, 2026
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