Store sampling context in the AsyncContextFrame directly#255
Store sampling context in the AsyncContextFrame directly#255
Conversation
Overall package sizeSelf size: 1.77 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | source-map | 0.7.6 | 185.63 kB | 185.63 kB | | pprof-format | 2.2.1 | 163.06 kB | 163.06 kB | | p-limit | 3.1.0 | 7.75 kB | 13.78 kB | | node-gyp-build | 3.9.0 | 8.81 kB | 8.81 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
bb7b1ea to
11f315e
Compare
BenchmarksBenchmark execution time: 2026-02-17 11:20:22 Comparing candidate commit 889d79f in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 91 metrics, 28 unstable metrics. scenario:profiler-idle-no-wall-profiler-18
|
067ef72 to
a425da6
Compare
30c38cb to
9f0aa90
Compare
9f0aa90 to
053743d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 053743d871
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bindings/profilers/wall.cc
Outdated
| Local<Object> WallProfiler::CreateContextHolder(Isolate* isolate, | ||
| Local<Context> v8Ctx, | ||
| Local<Value> value) { | ||
| auto wrap = | ||
| wrapObjectTemplate_.Get(isolate)->NewInstance(v8Ctx).ToLocalChecked(); | ||
| // for easy access from JS when cpedKey is an ALS, it can do |
There was a problem hiding this comment.
Guard createContextHolder when CPED is disabled
The new CreateContextHolder method unconditionally dereferences wrapObjectTemplate_ and calls ToLocalChecked(), but that template is only initialized when useCPED is true in the constructor (see the if (useCPED) block around lines 660–669). If a user creates a TimeProfiler with the default useCPED: false (or on a platform where CPED is unavailable) and calls createContextHolder(), this will hit an empty Persistent and trigger a V8 fatal error instead of a clean exception. Consider checking useCPED()/wrapObjectTemplate_.IsEmpty() and throwing a JS error similar to runWithContext to avoid a hard crash.
Useful? React with 👍 / 👎.
ba3946e to
8d8cbf2
Compare
8d8cbf2 to
8bf8759
Compare
01ebbd6 to
70343d3
Compare
time-profiler.ts uses it to pass an AsyncLocalStorage (ALS) as the key. This allows retrieval of context through the ALS, and can open a way for additional context features, like custom labels.
70343d3 to
5a9c1ae
Compare
What does this PR do?:
AsyncContextFrame(ACF) object in the CPED with a proxy that has an internal field for our sample context.AsyncLocalStorageas the key so the ACF map remains homogenous in its keys.Motivation:
Simplification and performance.
Simplification: Storing the sampling context directly in the ACF reduces the runtime complexity by not having a proxy object. Arguably, there's the added complexity of having the raw memory map reading code, but there's only so much complexity you can reduce and the remaining inherent complexity you can only push around from one place to another.
Performance: We also no longer need to ensure that the proxy object is updated every time a new async context is created, thus dd-trace-js no longer needs to instrument
AsyncLocalStorage.enterWith, eliminating the need to do any JS work in any callback on context creation or switches. I think this is pretty significant. See https://github.com/DataDog/dd-trace-js/tree/szegedi/acf-simple for the dd-trace-js side of changes.Additional Notes:
While storing the sampling context is now simpler, retrieving it from the signal handler actually got more complex, but that's a tradeoff we should make. Namely, in the signal handler now we need to chase pointers from current isolate to the hash table backing the ACF storage, and then also implement the hash table lookup-by-key. None of these data structures are published by V8, but we're defining equivalent ones that hold for the currently used V8 versions. They would also need to be updated if V8 internally changes them, but we have enough tests to detect if they no longer work.
How to test the change?
All existing CPED tests pass, and I've added some additional tests in
test-get-value-from-map-profiler.ts.