00:00 | <James M Snell> | As Justin suggests, we can easily replace
To achieve the use case |
00:00 | <James M Snell> | The idea with AsyncContext.snapshot() is to just eliminate the extra boilerplate of that wrap tho |
00:15 | <James M Snell> | While AsyncResource has other use cases relating to async_hooks, the only one that is relevant here is capturing the context on object creation and being able to call into that multiple times |
11:01 | <Chengzhong Wu> | https://github.com/legendecas/proposal-async-context/issues/19 |
15:15 | <littledan> | Sure, so since this is not expressiveness but rather ergonomics, it would help if someone could point at actual use cases so I could understand why this is worth it |
16:05 | <James M Snell> | Take a look at the piscina library. That was the origin of Node.js' EventEmitterAsyncResource. It shows a practical use case. Another useful case, look at HTMLRewriter change here https://github.com/cloudflare/workerd/pull/282/commits/0ffd4efd1914b428639499517e0177bd843a6583 |
16:07 | <James M Snell> | That's not using AsyncResource but the effect is the same. We capture the context frame once and enter it each time the registered callbacks are called |
16:08 | <James M Snell> | Or, we can use AsyncResource to run in the context where the handlers are registered. |
16:08 | <James M Snell> | It gives a good amount of flexibility |
18:02 | <Justin Ridgewell> | |
18:04 | <Justin Ridgewell> | I think in this particular case, using this.callback = AsynContext.wrap(callback) would have been appropriate |
18:04 | <Justin Ridgewell> | But snapshot() would be helpful for capturing the context used for multiple callbacks, instead of wrapping each individually |
18:05 | <James M Snell> | Yep absolutely. We had no other choices at the time. |
18:06 | <James M Snell> | That's why I also point at the HTMLRewriter example. It has the multiple callbacks so provides a good contrast |
18:15 | <James M Snell> | Wrapping each individual callback is also very expensive in node.js' current model since each is a separate AsyncResource that copies the context |
18:16 | <James M Snell> | That won't the case in the revised model, of course, but currently it's pretty expensive to wrap each individual callback so using AsyncResource is less costly but isn't really quite right in most cases |
18:19 | <Chengzhong Wu> | I've been poking the current implementation to adopt the revised model, based on the current async_hooks. |
18:20 | <Chengzhong Wu> | The performance improvements can be significant |
18:22 | <Chengzhong Wu> | it is still worthwhile to update the current implementation, since we won't get the new v8 apis on older LTS lines |
18:24 | <James M Snell> | Yeah I think once we get a few tweaks in place we should be able to transition fairly easily while keeping the old releases on the asynchooks model |
18:27 | <Chengzhong Wu> | yeah, maybe I can submit my micro-benchmarks first -- I still need more time to cleanup my improvements. |
18:34 | <Chengzhong Wu> | but, right, with the revised model, wrap and snapshot should have no performance differences. |