Only call getsectiondata() once per image.
#734
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR improves the performance of test discovery by only calling `getsectiondata()` once per loaded image instead of twice. It also refactors the C++ platform-specific enumerator function to take a new structure, `SWTSectionBounds`, instead of multiple arguments. This lets us use a C++ range-for-loop (`for (const auto& record : sectionBounds)`) instead of a raw C for-loop, reduces register traffic on Windows, and is all around a bit more object-oriented (which I hope is an improvement, yeah?) A non-scientific comparison at my desk shows that, before the change, **cold** test discovery on macOS took an average of 0.0376 seconds. After the change, the average was 0.0217 seconds for an approximate 1.73x speedup.
grynspan
added
enhancement
grynspan
requested review from
stmontgomery,
dennisweissmann,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
|
@swift-ci test |
grynspan
commented
Sep 26, 2024
| @@ -392,27 +416,27 @@ static void enumerateTypeMetadataSections(const SectionEnumerator& body) { | |||
| // Look in all the loaded modules for Swift type metadata sections and store | |||
| // them in a side table. | |||
| // | |||
| // This two-step process is less algorithmically efficient than a single loop, | |||
| // but it is safer: the callback will eventually invoke developer code that | |||
| // This two-step process is more complicated to read than a single loop would | |||
There was a problem hiding this comment.
Technically it's O(n) either way, so it's not algorithmically more complex, but it is longer code.
… address anyway (and I knew that, what was I doing?)
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
SeanROlszewski
approved these changes
Sep 26, 2024
| #else | ||
| using SWTMachHeaderList = SWTVector<const mach_header *>; | ||
| #endif | ||
|
|
||
| /// Get a copy of the currently-loaded Mach headers list. |
There was a problem hiding this comment.
Any of these docs need to be updated to reflect the new "section bounds" instead of "headers lists" terminology?
There was a problem hiding this comment.
Yeah I'll do a quick pass before I merge.
stmontgomery
approved these changes
Sep 26, 2024
|
@swift-ci test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves the performance of test discovery by only calling
getsectiondata()once per loaded image instead of twice. It also refactors the C++ platform-specific enumerator function to take a new structure,SWTSectionBounds, instead of multiple arguments. This lets us use a C++ range-for-loop (for (const auto& record : sectionBounds)) instead of a raw C for-loop, reduces register traffic on Windows, and is all around a bit more object-oriented (which I hope is an improvement, yeah?)A non-scientific comparison at my desk shows that, before the change, cold test discovery on macOS took an average of 0.0376 seconds. After the change, the average was 0.0217 seconds for an approximate 1.73x speedup.
Checklist: