-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improving Performance of ALS #1
Comments
@Darkripper214 Sure.
|
Using the test from the node issue I see that ALS has the impact:
A modified version for cls-hooked shows:
With another modification for the stack approach within a run() and one get every 100 async I get a 30% penalty. 7M/s In real-live I estimate not more than 1 get per 100 async A few percent were saved by eliminating the string operations:
|
The stack approach is not better than ALS when getContext is used twice (thats the least number of usages. One set, one get) |
@Darkripper214 The problem is that the stack is flooded with 'fn' even when having awaits everywhere. In average there are 46.
When doing the stack-trace on the second nest level the average stack depths is 52. Also the stack is not linear and the wrong context is picked. So forget that approach. Its not working. |
I think this library is about there, I still have some small idea to tweak some of the stuff, but not sure if it works. https://github.com/Darkripper214/ALS-CLS-Benchmark You can do |
FWIW, in my use-case, this package, ALS-Context was only marginally faster than cls-hooked on node16 but there were some corner cases that needed working around while cls-hooked worked out of the box. I believe it's a result of a similar bind/bindemitter type issue related to the AsyncLocalStorage. In particular, the one case I identified was with callbacks from the redis client. Inside the async/callback context after doing a redis fetch, the ALS context was lost. Rather than try to look for other such cases, I decided to stick with cls-hooked for now. |
Continuing from Jeff-Lewis/cls-hooked#63
@SimonX200
Could you share the code that you're mentioning?
I'm actually rewriting cls-hooked (to support for node v8 till the introduction of ALS). From what I understand, ALS actually keep the
store
insideAsyncResource
, and it could be retrieved inside async operation easily withasync_hooks.executionAsyncResource()
. Is that what you meant?Sadly
async_hooks.executionAsyncResource()
is introduce about the same time asALS
, so I've got to rewrite this logic without help from the internal bindings fromv8
.The text was updated successfully, but these errors were encountered: