Store the ElementRareDataVector pointers in AdditionalBytes. [chromium/src : main]
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
I'm aware that this feels a bit complicated; I'm open to ideas for how to improve the API if you can think of anything. I thought of maybe taking in a ElementRareData*& so that the setters could just update the pointers themselves, but Node stores the member compressed, so then they'd have to take in Member<ElementRareData>& instead and that didn't feel ideal…
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
I'm aware that this feels a bit complicated; I'm open to ideas for how to improve the API if you can think of anything. I thought of maybe taking in a ElementRareData*& so that the setters could just update the pointers themselves, but Node stores the member compressed, so then they'd have to take in Member<ElementRareData>& instead and that didn't feel ideal…
Not using UNSAFE_BUFFERS would be ideal, but I did enjoy seeing memcpy() for the first time since school :)
I like how UnpackAndRefresh abstracts away a lot of the std::pair ugliness. I wonder if we should also try to further abstract away how some fields have the std::pair listed in the header and some don't - it might make it confusing to add a new field later, but I guess its probably fine as-is.
#endifSo this copies the memory of the members of the old ElementRareDataVector into the new ElementRareDataVector, right? And we don't need to call constructors or destructors on all those members?
Related details
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 2 comments![]( "Open in Gerrit")
I see we're failing some tests, so I'll need to figure out in which patch that starts happening. Hopefully, it's nothing large.
So this copies the memory of the members of the old ElementRareDataVector into the new ElementRareDataVector, right? And we don't need to call constructors or destructors on all those members?
Indeed; it copies them over, but note that this is the Member<>, not the element itself. So we're just copying pointers.
Now, normally, you cannot copy Member<> with memcpy; there needs to be a barrier in case the object is in the middle of a trace from another thread. But constructors are different, since the object cannot be referenced from anywhere (nothing knows its address until the constructor returns). So there it's safe.
Given that this move constructor is called pretty rarely and thus isn't all that performance-sensitive, and mlippautz pointed out that VectorTypeOperations::Move is probably applicable here, so I'll probably switch to that for some less subtlety.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
Joey ArharI'm aware that this feels a bit complicated; I'm open to ideas for how to improve the API if you can think of anything. I thought of maybe taking in a ElementRareData*& so that the setters could just update the pointers themselves, but Node stores the member compressed, so then they'd have to take in Member<ElementRareData>& instead and that didn't feel ideal…
Not using UNSAFE_BUFFERS would be ideal, but I did enjoy seeing memcpy() for the first time since school :)
I like how UnpackAndRefresh abstracts away a lot of the std::pair ugliness. I wonder if we should also try to further abstract away how some fields have the std::pair listed in the header and some don't - it might make it confusing to add a new field later, but I guess its probably fine as-is.
Hm, can you point out some examples of the difference? Generally Ensure\*() needs the pair and Set\*() doesn't, but I could have forgotten something?
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
Joey ArharI'm aware that this feels a bit complicated; I'm open to ideas for how to improve the API if you can think of anything. I thought of maybe taking in a ElementRareData*& so that the setters could just update the pointers themselves, but Node stores the member compressed, so then they'd have to take in Member<ElementRareData>& instead and that didn't feel ideal…
Steinar H GundersonNot using UNSAFE_BUFFERS would be ideal, but I did enjoy seeing memcpy() for the first time since school :)
I like how UnpackAndRefresh abstracts away a lot of the std::pair ugliness. I wonder if we should also try to further abstract away how some fields have the std::pair listed in the header and some don't - it might make it confusing to add a new field later, but I guess its probably fine as-is.
Hm, can you point out some examples of the difference? Generally Ensure\*() needs the pair and Set\*() doesn't, but I could have forgotten something?
Yes Ensure needs the pair, but it now it feels like it has more boilerplate and loses the [[nodiscard]] on the new ElementRareData, right? I guess maybe there isn't really much more boilerplate other than writing out the std::pair type.
I guess its fine though, the "Ensure" pattern is still very useful for a lot of fields.
Steinar H GundersonSo this copies the memory of the members of the old ElementRareDataVector into the new ElementRareDataVector, right? And we don't need to call constructors or destructors on all those members?
Indeed; it copies them over, but note that this is the Member<>, not the element itself. So we're just copying pointers.
Now, normally, you cannot copy Member<> with memcpy; there needs to be a barrier in case the object is in the middle of a trace from another thread. But constructors are different, since the object cannot be referenced from anywhere (nothing knows its address until the constructor returns). So there it's safe.
Given that this move constructor is called pretty rarely and thus isn't all that performance-sensitive, and mlippautz pointed out that VectorTypeOperations::Move is probably applicable here, so I'll probably switch to that for some less subtlety.
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 2 comments![]( "Open in Gerrit")
Joey ArharI'm aware that this feels a bit complicated; I'm open to ideas for how to improve the API if you can think of anything. I thought of maybe taking in a ElementRareData*& so that the setters could just update the pointers themselves, but Node stores the member compressed, so then they'd have to take in Member<ElementRareData>& instead and that didn't feel ideal…
Steinar H GundersonNot using UNSAFE_BUFFERS would be ideal, but I did enjoy seeing memcpy() for the first time since school :)
I like how UnpackAndRefresh abstracts away a lot of the std::pair ugliness. I wonder if we should also try to further abstract away how some fields have the std::pair listed in the header and some don't - it might make it confusing to add a new field later, but I guess its probably fine as-is.
Joey ArharHm, can you point out some examples of the difference? Generally Ensure\*() needs the pair and Set\*() doesn't, but I could have forgotten something?
Yes Ensure needs the pair, but it now it feels like it has more boilerplate and loses the [[nodiscard]] on the new ElementRareData, right? I guess maybe there isn't really much more boilerplate other than writing out the std::pair type.
I guess its fine though, the "Ensure" pattern is still very useful for a lot of fields.
You're right, there's a number of Ensure\*() that have missing [[nodiscard]]. OTOH we don't really fear that people are ignoring the result value, we mostly fear that they are ignoring the second half of it (and nodiscard doesn't help us there). Do you have any good suggestions?
And yes, it's really verbose. It's possible that we could wrap it in some kind of struct where the only way to get out the result is to also feed it an ElementRareDataVector\*& to update, but that sounds… weird.
Uploading to rerun tests, I fixed a number of (thankfully local) issues but not sure if I got them all.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
Joey ArharI'm aware that this feels a bit complicated; I'm open to ideas for how to improve the API if you can think of anything. I thought of maybe taking in a ElementRareData*& so that the setters could just update the pointers themselves, but Node stores the member compressed, so then they'd have to take in Member<ElementRareData>& instead and that didn't feel ideal…
Steinar H GundersonNot using UNSAFE_BUFFERS would be ideal, but I did enjoy seeing memcpy() for the first time since school :)
I like how UnpackAndRefresh abstracts away a lot of the std::pair ugliness. I wonder if we should also try to further abstract away how some fields have the std::pair listed in the header and some don't - it might make it confusing to add a new field later, but I guess its probably fine as-is.
Joey ArharHm, can you point out some examples of the difference? Generally Ensure\*() needs the pair and Set\*() doesn't, but I could have forgotten something?
Steinar H GundersonYes Ensure needs the pair, but it now it feels like it has more boilerplate and loses the [[nodiscard]] on the new ElementRareData, right? I guess maybe there isn't really much more boilerplate other than writing out the std::pair type.
I guess its fine though, the "Ensure" pattern is still very useful for a lot of fields.
You're right, there's a number of Ensure\*() that have missing [[nodiscard]]. OTOH we don't really fear that people are ignoring the result value, we mostly fear that they are ignoring the second half of it (and nodiscard doesn't help us there). Do you have any good suggestions?
And yes, it's really verbose. It's possible that we could wrap it in some kind of struct where the only way to get out the result is to also feed it an ElementRareDataVector\*& to update, but that sounds… weird.
If there was a way to use templates to minimize the amount of places we are missing [[nodiscard]] and have to write the std::pair that would be cool, but if not thats fine. lgtm as-is
Related details
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |