feat: Use callbacks for evaluation hotpath. #234
Conversation
| this.bucketer = new Bucketer(platform.crypto); | ||
| } | ||
|
|
||
| async evaluate(flag: Flag, context: Context, eventFactory?: EventFactory): Promise<EvalResult> { |
There was a problem hiding this comment.
For unit test compatibility I kept an async version.
kinyoklion
requested review from
louis-launchdarkly and
yusinto
as code owners
yusinto
left a comment
yusinto
left a comment
There was a problem hiding this comment.
Using callbacks increases complexity make this more difficult to maintain. I feel nervous we are going back to callbacks, simultaneously understand the need for performance. I would like to request in the future we further investigate improving performance with async/await so we can have a win-win.
Are you able to tag this commit "revert to callbacks" so it's easily detectable for future reference please? Thank you.
Well, we never used callbacks in this implementation. So it isn't really reverting, and it isn't 100% equivalent to the callback implementation from the old implementation. I can put callbacks in the title though. |
kinyoklion
changed the title
kinyoklion
force-pushed
the
rlamb/performance-experimentation
branch
from
e9175ee to
8a15e15
Compare
| } | ||
|
|
||
| async allFlagsState( | ||
| allFlagsState( |
There was a problem hiding this comment.
Because we have to make a manual promise anyway removing the async here may save a small amount of overhead.
| `Error for feature flag "${flag.key}" while evaluating all flags: ${res.message}`, | ||
| ), | ||
| ); | ||
| const doEval = (valid: boolean) => |
There was a problem hiding this comment.
Some refactoring here to allow the callback version of the initialized function for the sore.
| const clientOnly = !!options?.clientSideOnly; | ||
| const detailsOnlyIfTracked = !!options?.detailsOnlyForTrackedFlags; | ||
|
|
||
| allAsync( |
There was a problem hiding this comment.
This is the main change. Evaluate the flags concurrently using promises.
This isn't strictly a win for performance, but it does do a couple things.
1.) It should be a consistent win for performance when using persistent stores.
2.) It lowers the risks of stack overflows with the new callback implementation.
3.) It changes the execution behavior to be more like the old SDK. This isn't necessarily positive or negative, but it does have that effect.
This PR changes the evaluation to use callbacks instead of async. In the case where there is no IO there are many cases where execution will just by recursive without being deferred in the task system.
Recursion does have its limits for collections though, large collections do use an immediately resolved promise to prevent the iteration from causing a stack overflow.
Generally speaking the callback depth here should be similar to that in the previous implementation, so it should not cause problems. Though I prefer greatly the async approach in regards to this.