Allocate CtxWrap and OtelThreadCtxRecord together in memory - #402
Allocate CtxWrap and OtelThreadCtxRecord together in memory#402szegedi wants to merge 3 commits into
Conversation
Overall package sizeSelf size: 2.55 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.3.1 | 504.33 kB | 504.33 kB | | source-map | 0.8.0 | 185.66 kB | 185.66 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
0d27c43 to
6acf591
Compare
This allows us to store the pointer to OtelThreadCtxRecord in the JS wrapper's internal field 0 so the reader is never exposed to CtxWrap and can read the record with one pointer indirection fewer.
0a4c2a1 to
475868f
Compare
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 475868f31f
ℹ️ 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".
| // weak handle and so cancels the WeakCallback V8 would otherwise fire on | ||
| // the freed block. | ||
| CtxWrap::Destroy(self); | ||
| self = new_self; |
There was a problem hiding this comment.
nit: at first it looks like a dead store, a (short) comment to explain it is needed because of the previous defer would be helpful.
There was a problem hiding this comment.
I was actually just thinking about this this morning and I figured we don't even need to do this:
encoding_guards against reentrancy, but all reentrancy can only happen withinEncodeAttrs. Once that returns, we might as well flipencoding_back to false and not deal with any of this further down the line.- Even if we keep the
deferblock, all reentrant calls would still be checking oldself.encoding_sonew_self.encoding_is never read here thus we might as well constructnew_selfwithencoding_ = falseand not reassignself.
Does this sound valid to you?
| return; | ||
| } | ||
|
|
||
| // Doesn't fit. Reallocate with geometric growth with cap. |
There was a problem hiding this comment.
nit: maybe extract this whole part into a separate function that returns the new CtxWrap
There was a problem hiding this comment.
Okay, thought about it and I wouldn't split the function like that. What might make more sense to me is to split the code into JS (V8 and Node) part (arg validation, reentrancy guard and few more concerns) that stays here in Append and then make a separate AppendEncoded that only deals with the memory mechanics of the native part (either in-place or growing.) E.g. Append would end with:
if (!self->AppendEncoded(args.This(), appended)) {
isolate->ThrowError("allocation failed");
}
and we'd have:
bool AppendEncoded(Local<Object> holder, const std::vector<uint8_t>& appended);
Let me add this as a separate commit and then I'll let you review the result.
|
|
||
| // Floor on the attrs_data capacity of a freshly allocated record. Sized so | ||
| // the total allocation is one 64-byte cache line — matching the OTEP-4947 | ||
| // the record itself is one 64-byte cache line — matching the OTEP-4947 |
There was a problem hiding this comment.
nit: I would remove the cache line mention because nothing is done to ensure the record starts at a cache line boundary. Moreover with calloc alignment (16 bytes) and record offset (40 bytes), I don't think it's actually possible to make record starts on a cache line boundary.
| // immediately after the object's own fields. Both record() and FromRecord() | ||
| // are defined in terms of it. | ||
| constexpr size_t RECORD_OFFSET = sizeof(CtxWrap); | ||
| static_assert(RECORD_OFFSET % alignof(OtelThreadCtxRecord) == 0, |
There was a problem hiding this comment.
nit: RECORD_OFFSET should also satisfy v8 aligned pointer requirementsfor SetAlignedPointerInInternalField (which it currently does since it's the same as OtelThreadCtxRecord: 2-byte alignment)
2 participants
What does this PR do?:
Allocates
CtxWrapandOtelThreadCtxRecordtogether in memory.This allows us to store the pointer to
OtelThreadCtxRecordin the JS wrapper's internal field 0 so the reader is never exposed toCtxWrapand can read the record with one pointer indirection fewer. Internally we can still find theCtxWrapby subtracting a fixed offset from theOtelThreadCtxRecordpointer.Motivation:
By storing the
OtelThreadCtxRecorddirectly in the JS object's internal field, an external reader can reach it directly from there instead of having to traverseCtxWraptoo.CtxWrapbecomes a purely internal implementation detail that the reader needs to know nothing about.Jira: PROF-15807