Update MethodDictionary rebuild to speed up trait recompilation - #19870
Update MethodDictionary rebuild to speed up trait recompilation#19870jecisc wants to merge 43 commits into
Conversation
…e way to reject some selectors
For now one is failing
Gabriel-Darbord
left a comment
Gabriel-Darbord
left a comment
There was a problem hiding this comment.
Thanks for this PR, this is great for Moose where traits are omnipresent :)
I have a few technical and quality suggestions.
I can't really comment on semantics and validity, but I don't see any blatant issue there.
| removedSelectors := self methodDict keys reject: [ :aSelector | | ||
| (compilationInfos anySatisfy: [ :compilationInfo | compilationInfo selector = aSelector ]) or: [ self isSelectorToKeep: aSelector ] ]. | ||
| modified := modified | removedSelectors isNotEmpty. | ||
| removedSelectors do: [ :aSelector | | ||
| self methodDict removeKey: aSelector. | ||
| self removeFromProtocols: aSelector ]. | ||
| self methodDict removeKey: aSelector. | ||
| self removeFromProtocols: aSelector ]. |
There was a problem hiding this comment.
removedSelectors seems like an unnecessary intermediate collection:
self methodDict keys do: [ :aSelector |
((compilationInfos anySatisfy: [ :compilationInfo | compilationInfo selector = aSelector ]) or: [ self isSelectorToKeep: aSelector ]) ifTrue: [
modified := true.
self methodDict removeKey: aSelector.
self removeFromProtocols: aSelector ] ].Explicitly use keys do: instead of keysDo: to avoid modification during iteration.
You could also try swapping conditions to check isSelectorToKeep: first to be potentially faster.
There was a problem hiding this comment.
I did not really check this part of the code since it did not appear in the profiler. I'll try to check soon :)
| nextPutAll: '>>'; | ||
| print: selector. | ||
|
|
||
| self compiledMethod ifNotNil: [ :cm | cm isRequired ifTrue: [ aStream nextPutAll: ' - Explicit requirement' ] ]. |
There was a problem hiding this comment.
If memoized and not afraid of concurrency (compiledMethod could change between nil check and block execution, but I don't think that's a concern for this data structure): cm isRequired -> self isExplicitRequirement
|
@jecisc let us know when this is ready. |
Co-authored-by: Gabriel Darbord <[email protected]>
Co-authored-by: Gabriel Darbord <[email protected]>
3 participants
This PR brings multiple speed up for large trait recompilation.
Recompiling large traits can be really slow because it needs to rebuild the method dictionary of each of the trait user.
This was done by iterating over the selectors of the trait composition and installing a copy of the method associated in the user. But we traversed multiple time the trait composition by doing this.
Now I build a compilation info object containing all the info directly preventing multiple visits of the trait composition.
Most of the time we still spend in the rebuild is spend in recompiling the methods if the user has a slot. The installation time of the method is much shorter.
For comparison, in a vanilla Moose image it was taking 72sec to add a slot to TEntityMetaLevelDependency. With this change it drops to 27sec.
There might be ways to speed up a little more operations, I might explore this later.