Skip to content

Comments

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

Merged
woodruffw merged 2 commits intomasterfrom
open-optimization
Feb 26, 2016
Merged

Optimization: Replace control-flow-by-exception with magic testing and incremental reads.#22
woodruffw merged 2 commits intomasterfrom
open-optimization

Conversation

@woodruffw
Copy link
Member

@woodruffw woodruffw commented Feb 18, 2016

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

@woodruffw woodruffw self-assigned this Feb 18, 2016
@woodruffw woodruffw mentioned this pull request Feb 18, 2016
Closed
@woodruffw woodruffw force-pushed the open-optimization branch 2 times, most recently from a87e629 to 128f119 Compare February 18, 2016 22:01
@woodruffw
Optimization: Replace control-flow-by-exception with magic testing
60aab61 
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.
@woodruffw
Testing: Add tests for new MachO.open
ee8906c 
Ensure that MachO.open raises all expected exceptions.
@woodruffw
Copy link
Member Author

woodruffw commented Feb 22, 2016

@UniqMartin Mind doing a quick lookover of this?

@UniqMartin
Copy link
Contributor

UniqMartin commented Feb 25, 2016

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

woodruffw commented Feb 25, 2016

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 woodruffw mentioned this pull request Feb 25, 2016
Open
woodruffw added a commit that referenced this pull request Feb 26, 2016
@woodruffw
Merge pull request #22 from Homebrew/open-optimization
ec64d0c 
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.

Reviewers

No reviews

Assignees

@woodruffw woodruffw

Labels

enhancement in progress optimization outdated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@woodruffw @UniqMartin