Speedup item::best_pocket ~100% (2x as fast) and misc other wins too #55070
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.
Summary
None
Purpose of change
Profiling of unit tests (esp
starting_items
) after recent changes from @BrettDong showed some weird inefficiencies. Besides the usual stdlib inefficiencies with copying instead of moving or referencing, there was what looked like a glaring 2x issue inbest_pocket
. Code at the top was callingcan_contain( it )
and exiting 'early' if it returned false. That function essentially just loops through all the contents callingcan_contain( it )
on each one. That is not cheap, though, and the subsequent loop repeats basically the same calls on the same contents but with more logic around it. This doesn't seem like a true fast early exit, just wasted cycles, and accounts for ~15-20s out of 240s of test runtime forstarting_items
tests. Plus it's live code.Describe the solution
Delete the probably redundant
can_contain( it )
call. Fix some list insert calls to be splices, and use a reference to astd::map
instead of a copy when callingmade_of()
.Describe alternatives you've considered
Testing
Tests still pass.
Additional context
After/Before of
starting_items
test with these changes @ 100 samples/second shows ~1800 sample reduction in test runtime, basically all fromCharacter::migrate_items_to_storage
without cost shift to elsewhere.