Repository navigation
perf: skip evaluation events nobody is subscribed to - #387
Conversation
Every isEnabled/getVariant call enqueues a ToggleEvaluated task on the shared UnleashScheduledExecutor, even when the configured subscriber doesn't implement toggleEvaluated. The enqueue takes the executor's queue lock, allocates a FutureTask plus queue and condition nodes and wakes a worker, so all evaluating threads end up contending on one lock. Every method on UnleashSubscriber is a default no-op, so EventDispatcher can work out once, at construction, whether an event type reaches anything: an event is skipped only when the subscriber inherits the default for both on(UnleashEvent) and the specific callback that event publishes to. Only ToggleEvaluated and ImpressionEvent are gated, every other event type always dispatches, and anything that cannot be resolved counts as overridden so no callback is ever silently dropped. DefaultUnleash checks isSubscribedTo before constructing the event, which also skips the UUID.randomUUID() in the ImpressionEvent constructor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Hi there @Meemaw 👋 Thanks a lot for the contribution, let us take a look at the PR and get back to you. |
| if (eventDispatcher.isSubscribedTo(IsEnabledImpressionEvent.class) | ||
| && response.map(r -> r.impressionData).orElse(false)) { | ||
| eventDispatcher.dispatch(new IsEnabledImpressionEvent(toggleName, enabled, context)); | ||
| } |
There was a problem hiding this comment.
I understand your argument: you avoid the allocation of an event that no one will receive by guarding dispatch here in isEnabled instead of inside of dispatch.
My main concern around that is the breaking of an atomic operation. Clients rely on the convention of "you have to call isSubscribedTo before calling dispatch", which is a convention. And conventions tend to be forgotten unless enforced.
I'd advise that we move this guard inside the EventDispatcher and that we keep it private. The benefits of the maintainability of that, I think, far exceed the shavings in memory footprint. You're already kind of doing it for shouldDispatch, so it should be okay.
There was a problem hiding this comment.
Agreed on keeping the guard inside EventDispatcher — dropped isSubscribedTo. To avoid paying for event construction (incl. UUID.randomUUID() for impression events) when nobody listens, EventDispatcher now exposes dispatchToggleEvaluated / dispatchIsEnabledImpression / dispatchVariantImpression, which check and build atomically. JMH (interleaved, 3 rounds): all PR variants are ~25–35% faster than main; guarding inside dispatch(UnleashEvent) alone costs ~150–180 ns/call on impression-data flags vs. the typed methods.
Drop the public isSubscribedTo so callers can't forget to check it. Per-evaluation events now go through typed dispatch methods that take the event's fields and only construct the event when something will receive it, keeping the evaluation path allocation-free for events nobody listens to. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-project-automation
Bot
moved this from In Progress
to Approved PRs
in Issues and PRs
|
@ppicom Thanks. Possible to get this released? |
|
On it! |
We call
isEnabledon hot request paths, and noticed that every call schedules a task on the shared executor to publish aToggleEvaluatedevent whether or not anything is listening. With several threads evaluating at once they all pile up on that one queue lock.EventDispatcheralready knows the subscriber at construction time, and every method onUnleashSubscriberis a default no-op, so it can work out up front whether an event type will reach anything. If the subscriber inherits the default for bothon(UnleashEvent)and the callback that event publishes to, there is nothing to deliver and the dispatch is skipped.Only
ToggleEvaluatedandImpressionEventare gated — those are the two that fire per evaluation. Everything else (onReady,togglesFetched, errors, metrics, backups) dispatches exactly as before. If the check can't be resolved for any reason it counts as overridden and we dispatch, so no callback is ever silently dropped.DefaultUnleashalso asks before constructing the event, which skips theUUID.randomUUID()in theImpressionEventconstructor.Numbers
JMH on
DefaultUnleash.isEnabled, bootstrapped toggles, polling and metrics off. JDK 21,-XX:ActiveProcessorCount=6, yggdrasil-engine 1.0.3. The subscriber here only implementsonReady:With no subscriber configured at all (the default
NoOpSubscriber) it goes 1897 -> 1480 ns at one thread, so -22%.Worth saying plainly: the six-thread rows are noisier than they look. On the machine I ran this on, most of the remaining time sits inside the native engine, which serialises on its own lock, so the Java-side win is partly hidden there. Happy to rerun somewhere else if that would help. The benchmark harness isn't part of this PR — I can push it if you'd like it tracked.
Notes
getClass() == NoOpSubscriber.classcheck would have been simpler, but it misses a case we hit in practice: a subscriber that exists only to react toonReady(refreshing some config after the first fetch) still pays the full per-evaluation cost. That's what prompted this.isSubscribedTois new public API onEventDispatcher. If you'd rather not expose it, the gate insidedispatchon its own gets most of the win — it just doesn't avoid the event allocation.EventDispatcherTestcovers the default subscriber,onReady-only,toggleEvaluated-only,impression-only, catch-allon(), aNoOpSubscribersubclass that overrides a callback, and non-evaluation events.🤖 Generated with Claude Code