Avoid a set per combineWith and cache TransitionFunctionImpl's hash - #334
Merged
Merged
Conversation
Follow-up to #281. combineWith built a LinkedHashSet of both functions' state change statements on every call, only to throw it away again in the common case where the union is a single statement. It now checks whether the other function already has all of this function's statements (typically both have the same single one) and, if so, returns a function of the other's kind with the merged sequences; only a genuinely new combination builds an ImmutableSet, which CombinedTransitionFunctionImpl then takes over without copying. The result's statements, and the one getStateChangeStatement() returns, are the same as before. hashCode() hashed the whole sequence multimap on every call, while the weights are hashed over and over as keys of the automata's maps. The functions are immutable, so the hash is now computed once and cached. With compressed oops the extra int fits in the padding of a plain TransitionFunctionImpl.
This file contains hidden or 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
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.
Follow-up to #281, picking up the allocation concern from its review.
What changes
combineWithbuilt aLinkedHashSetof both functions' state change statements on every call, only to discard it in the common case where the union is a single statement. It now checks whether the other function already has all of this function's statements (typically both have the same single one) and, if so, returns a function of the other's kind with the merged sequences. Only a genuinely new combination builds anImmutableSet, whichCombinedTransitionFunctionImpltakes over without copying (ImmutableSet.copyOfon anImmutableSetis a no-op).hashCode()hashed the whole sequence multimap on every call, while weights are hashed repeatedly as keys of the automata's maps. The functions are immutable, so the hash is computed once and cached (the same racy-single-check idiom asString.hashCode). With compressed oops the extraintfits in the existing padding of a plainTransitionFunctionImpl; only the rare combined instance grows by 8 bytes.Behaviour is unchanged: the result has the same set of state change statements as before,
getStateChangeStatement()returns the same statement, and combining stays commutative and idempotent.TransitionFunctionImplTestgains an assertion for the shortcut direction and for hash equality of the commutative results.Testing (
-DtestSetup=Soot)IteratorHasNextTesttest1/test2/test4, which fail identically ondevelop.FileMustBeClosedTest(49 tests): 71.1 s on this branch vs 73.7 s ondevelop, one run each, so within noise. No regression, but no claimed speed-up either.🤖 Generated with Claude Code