Optimization: removed level of indirection for SubRuleContext. #173
Conversation
|
hey @svladykin is this ready to review? its' been in draft state which has me confused on the state of the PR. |
| private final Map<Object, Set<Double>> nameToIds = new ConcurrentHashMap<>(); | ||
| private final Map<Double, Object> idToName = new ConcurrentHashMap<>(); | ||
| private final Map<Object, Set<SubRuleContext>> nameToContext = new ConcurrentHashMap<>(); | ||
| private long nextId; |
There was a problem hiding this comment.
Should this be AtomicLong ?
There was a problem hiding this comment.
From what I see the only place where it is called is GenericMachine.addPatternRule() under synchronized(this), so AtomicLong does not make much sense.
| @@ -175,12 +175,11 @@ boolean isEmpty() { | |||
| * @param pattern The pattern used by the sub-rule to transition to this NameState. | |||
| * @param isTerminal True indicates that the sub-rule is using pattern to match on the final event field. | |||
| */ | |||
| void addSubRule(final Object rule, final double subRuleId, final Patterns pattern, final boolean isTerminal) { | |||
| void addSubRule(final Object rule, final SubRuleContext subRuleId, final Patterns pattern, final boolean isTerminal) { | |||
There was a problem hiding this comment.
wondering if here and elsewhere in the class if rule can be fetched from subRuleId instead
There was a problem hiding this comment.
This is possible, but looks like if we do that everywhere the patch will be much larger, at the same time I don't see much improvement from this change. Either option is fine with me.
There was a problem hiding this comment.
I think that's an acceptable next step for this PR, though digging into JMH variances would be better place to focus right now.
|
@baldawar I did not like the |
|
Rebased on top of main to check JMH throughput numbers. |
|
odd, perf tests are mixed bag when compared to https://github.com/aws/event-ruler/actions/runs/10805542552/job/30018080464 . Is it because of most of the time is spent in other parts of the code (my recent tests showed |
|
Yes, I don't see any throughput improvement as well. We can either consider this patch to be just a code cleanup or just close it and stop spending time on it. I'm good with either option. I also noticed that json parsing takes around 60% of the time for simple rules, which makes me think that using custom binary format instead of json could help a lot. |
|
FWIW, Quamina has a custom hand crafted JSON parser for events, and the benefits of that were huge. But didn't bother for rule parsing. |
|
useful bookmark for the custom JSON parser :
Ruler's tests make parsing look as the worst offender because we keep adding / removing json entries to setup the tests. In the wild, most of the time ruler is limited by the time-spent doing array consistency checks, and parsing numbers. There's a fair amount of usage of exists and anything-but matchers which need cleanup (to follow how wildcard matcher was implemented). Optimizing JSON parsing will still be meaningful, but so far I've found most folks to be content with the current speed. For this change, I think its worth merging if we can add a test that shows SubRuleContext is now faster. Benchmark would be over kill but unit test would be good enough with bunch of for-loops similar to |
|
@svladykin let me know if you're still working on this. |
|
Yes, was a bit busy lately, will catch up this week. |
Description of changes:
Instead of
Doubleid useSubRuleContextdirectly everywhere.SubRuleContextid is only used internally for backward compatibility inequalsandhashCode(not sure if this is actually needed, but tests check that). The benchmarks are not stable (need JMH with forking, proper warmup and multiple longer iterations for individual benchmarks), but overall can see 3-8% in throughput improvement on some benchmarks with this easy fix.Using
longas id instead ofdoublebecauselongis typically faster and actually can produce more unique 64-bit patterns thandoublebecause forlongevery 64-bit pattern is valid while fordoubleit is not true (NaN).Changed only types but did not rename variables all over the place because it would be harder to review, can be done later as needed.
Benchmark / Performance (for source code changes):
Overall the becnhmark results are not conclusive across diffrent jvm versions: the same benchmark can be noticeably better or worse, from what I see from main branch history they are quite flaky. Will try to improve benhcmarks in a separate PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.