Allocate CtxWrap and OtelThreadCtxRecord together in memory - #402
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".
|
|
||
| // 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.
There was a problem hiding this comment.
that's a good point.
@scottgerring do you think it'd be worth trying to optimize for this?
We could use aligned_alloc to put the total allocation at a 64 byte boundary, but then in order for the record to be on a cache line boundary, we'd have to add 24 bytes of slack into the CtxWrap so it goes from 40 to 64 bytes. Our initial allocations become 128 bytes, 64-aligned, we always sacrifice 24 bytes.
Yet another possibility is to keep 16-aligned calloc but allocate enough extra space that we can find a 64-byte aligned start address for the record within it. Since we keep a capacity, if the multiple-of-64 happens to be earlier in the allocated space, what's behind can be added to initial capacity_ and opportunistically remain as usable space for the record to grow. That'll make our initial allocations always be somewhat bigger, though. Again, I didn't do the full math in my head but I expect this'd be 160 bytes (40+8+64+48), but some of those last 48 would be usable for record growth. Again, this is both more complicated and yields more surprising runtime variation (as records will have different initial capacities, needing reallocations at different sizes) so maybe not?
If we want to be both cheaper and whimsical we can just add 8 bytes of slack so record offset is 48 instead of 40, so with calloc the record has a 25% percent chance of being on a cache line boundary if calloc allocates at 16 modulo 64. (No, I'm not really being serious about this in case it isn't obvious 😉.)
Or we can just remove the cache line mention as Nicolas suggested.
There was a problem hiding this comment.
Alright, removed the mentioning of the cache line for now.
There was a problem hiding this comment.
I like the idea of over-allocating and searching for alignment, but I have no idea if the juice is worth the squeeze :D Could go wild microbenchmarking I s'pose but @nsavoire's "let's just not talk about it for now" seems pragmatic
| // 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)
3 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