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

Optimization: Replace control-flow-by-exception with magic testing and incremental reads. #22

Merged
merged 2 commits into from Feb 26, 2016

Conversation

woodruffw
Copy link
Member

Instead of attempting to open a file with MachOFile.newand catching
a FatBinaryError to attempt FatFile.new, read a few bytes and do some
magic/sanity tests. Once sanity is established, instantiate either a
MachOFile or a FatFile.

Tasks:

  • Add attr_accessor filename
  • Open and check the file directly in MachO.open
  • Unit tests

Reference: #7

and incremental reads.

Instead of attempting to open a file with MachOFile.new and catching
a FatBinaryError to attempt FatFile.new, read a few bytes and do some
magic/sanity tests. Once sanity is established, instantiate either a
MachOFile or a FatFile (or error out with a MagicError).

Additionally, stop truncated files (< 4 bytes) ahead of time with
a TruncationError.
Ensure that MachO.open raises all expected exceptions.
@woodruffw
Copy link
Member Author

@UniqMartin Mind doing a quick lookover of this?

@UniqMartin
Copy link
Contributor

Mind doing a quick lookover of this?

Looks good to me! (And sorry for being slow to respond!)

Are you intending to make changes to FatFile and MachOFile too, possibly even changing them in a way such that they don't have to read the entire file to do their work? I admit that shouldn't be such a pressing issue now that the MachO.open method is much more efficient (after this PR is merged), just wondering about your plans. 😃

@woodruffw
Copy link
Member Author

Looks good to me! (And sorry for being slow to respond!)

Thank you (and no worries)!

Are you intending to make changes to FatFile and MachOFile too, possibly even changing them in a way such that they don't have to read the entire file to do their work? I admit that shouldn't be such a pressing issue now that the MachO.open method is much more efficient (after this PR is merged), just wondering about your plans.

Yeah, this would be ideal. I probably won't get around to it in this PR, but it would ultimately be much nicer to spread these performance improvements around if and when users choose to create MachOFile and FatFile instances directly.

I'll merge this tonight!

woodruffw added a commit that referenced this pull request Feb 26, 2016
Optimization: Replace control-flow-by-exception with magic testing and incremental reads.
@woodruffw woodruffw merged commit ec64d0c into master Feb 26, 2016
@UniqMartin UniqMartin deleted the open-optimization branch June 15, 2016 18:16
@lock lock bot added the outdated label Jan 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants