Store sampling context in the AsyncContextFrame directly#255
Conversation
Overall package sizeSelf size: 1.87 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
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".
ba3946e to
8d8cbf2
Compare
8d8cbf2 to
8bf8759
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 889d79f2aa
ℹ️ 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".
* We now validate a bunch of invariants for the OrderedHashMap * This makes us bail out if anything looks wrong, but also requires fewer checks in the lookup.
1744b88 to
324c45b
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.