Small performance optimization#14820
Conversation
Performance improvements to ContinuousEffects layer processing: - Use HashSet for O(1) effect ID lookups in updateTimestamps - Replace stream with loop in filterLayeredEffects - Cache ability lookups across PT sublayer passes - Return static empty set from getAbility instead of allocating
|
Build failed with this error, doesn't reproduce when I run locally: [INFO] Stack trace [INFO] |
|
Last screenshot is little wrong - it’s show database warm up on first run (card scan), so ApplyEffect’s different is hard to read/analyse. You can run one of the performance unit test multiple time and collect total time to execute (or just run whole project tests) - ApplyEffects optimization must affect any of it. |
JayDi85
left a comment
JayDi85
left a comment
There was a problem hiding this comment.
Some ideas. Need farther research.
| // build a HashSet of previous effect IDs for O(1) lookups instead of O(n) linear scans | ||
| Set<UUID> prevIds = new HashSet<>(prevs.size()); | ||
| for (ContinuousEffect prev : prevs) { | ||
| prevIds.add(prev.getId()); |
There was a problem hiding this comment.
Must add additional check here on false result and raise error on it (to catch bad use cases with same effect id and research it).
| .filter(effect -> effect.hasLayer(layer)) | ||
| .collect(Collectors.toList()); | ||
| List<ContinuousEffect> filtered = new ArrayList<>(); | ||
| for (ContinuousEffect effect : effects) { |
There was a problem hiding this comment.
“Stream to for cycle” replacement is useless for performance optimization
|
|
||
| layer = filterLayeredEffects(activeLayerEffects, Layer.PTChangingEffects_7); | ||
| // pre-compute ability lookups for PT layer since we iterate 5 sublayers over the same effects | ||
| Map<UUID, Set<Ability>> ptAbilitiesCache = new HashMap<>(layer.size()); |
There was a problem hiding this comment.
I don’t like HashMap usage - it’s will generate random list in some use cases. If something depends on the effects orders then can fail. It’s better to use LinkedHashMap instead.
| for (ContinuousEffect effect : layer) { | ||
| Set<Ability> abilities = layeredEffects.getAbility(effect.getId()); | ||
| for (Ability ability : abilities) { | ||
| ptAbilitiesCache.put(effect.getId(), layeredEffects.getAbility(effect.getId())); |
There was a problem hiding this comment.
Single effects list calculation instead recalculate can lead to buggy use cases - if some of the effect added to the game after apply call then it will be missed from the list. I’m not sure - is that actual or not, need research.
|
Ok I reran the tests and generated profile SVGs Test Setup
You can see in the AFTER section more time is spent in JUNIT testing (SVGs are clickable to dive into details) Biggest improvement is from the abilities cache, but they all contribute. 2.6x reduction in wallclock time: BEFORE
## AFTER
|
2 participants
Summary
Performance improvements to
ContinuousEffectslayer processing:HashSetfor O(1) effect ID lookups inupdateTimestampsfilterLayeredEffectsgetAbilityinstead of allocatingIncludes regression tests.
Profiled with JDK Mission Control on MacOS Tahoe 26 (M1 Air)
Before (lots of time in ApplyEffect):
After (less time spent in ApplyEffect):
