[heap] Cleanup descriptor array trimming flags [v8/v8 : main]
Omer Katz (Gerrit)
Omer Katz removed a vote from this change![]( "Open in Gerrit")
Removed Commit-Queue+1 by Omer Katz <omer...@chromium.org>
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 1 comment![]( "Open in Gerrit")
Related details
- Dominik Inführ
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 1 comment![]( "Open in Gerrit")
First M152 canary is already out. I think it should be safe to land this clean up now.
Related details
- Dominik Inführ
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 1 comment![]( "Open in Gerrit")
I'll split the StrongDescriptorArray stuff to a separate CL and will resend later.
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
As discussed offline: Would be nice if we could land this in smaller chunks :)
Related details
- Omer Katz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 1 comment![]( "Open in Gerrit")
ptal.
I moved the StrongDescriptorArray cleanup to a followup CL.
I tried to split this CL even further, but I think it makes more sense to keep as is.
Related details
- Dominik Inführ
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 2 comments![]( "Open in Gerrit")
WriteBarrier::ForDescriptorArray(descriptors);Isn't this wrong? It tries to mark the descriptor array which won't work if this happens repeatedly?
In the spirit of a regular write barrier we should just have a write barrier for the individual slot here: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/descriptor-array-inl.h;l=297;drc=a81bc8561d938a2cfa58705d6d7c1121a2dd0f78?q=DescriptorArray::append&ss=chromium
?
WriteBarrier::ForDescriptorArray(to_replace);I don't understand why this is necessary. I guess it's fine to keep for now but I don't see why we need to keep `to_replace` alive here.
Related details
- Dominik Inführ
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 2 comments![]( "Open in Gerrit")
Isn't this wrong? It tries to mark the descriptor array which won't work if this happens repeatedly?
In the spirit of a regular write barrier we should just have a write barrier for the individual slot here: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/descriptor-array-inl.h;l=297;drc=a81bc8561d938a2cfa58705d6d7c1121a2dd0f78?q=DescriptorArray::append&ss=chromium
?
I looked into it. It's not wrong just leftover from a bygone era and now redundant. We already get write barrier for the newly appended descriptor in `DescriptorArray::Set` (I was surprised actually that `Relaxed_Store` includes a barrier for each write). This write barrier most likely was historically needed for making sure the whole array is marked when we append a new descriptor to the end. In an older version of V8, without the write barrier, you would try to trim the array and lost the newly appended descriptor. That's no longer the case.
I removed it.
I don't understand why this is necessary. I guess it's fine to keep for now but I don't see why we need to keep `to_replace` alive here.
I think the comment above explains why it was necessary. Specifically, "The old descriptors will not be trimmed in the mark-compactor, we need to mark all its elements", e.g. in case to_replace was already marked.
Since we now always mark all elements, this write barrier should not be needed anymore.
I removed it.
Related details
- Dominik Inführ
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 5 comments![]( "Open in Gerrit")
static inline void ForDescriptorArray(Tagged<DescriptorArray>);Do we still need this? It feels like the remaining uses aren't necessary but I could easily miss something.
void MarkCompactCollector::WeakenStrongDescriptorArrays() {Do we still need strong descriptor arrays? Strictly speaking we shouldn't be in the deserializer when we trim DescriptorArrays because we don't trim with a stack. It's a bit indirect though but this is similar to what we do for strings. I guess could e.g. CHECK that none of the LocalHeaps is in the deserializer to make sure this holds. If so, not for this CL obviously.
WriteBarrier::ForDescriptorArray(descriptors);This barrier here shouldn't be necessary, right? The set_instance_descriptors above should mark it.
Related details
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 4 comments![]( "Open in Gerrit")
static inline void ForDescriptorArray(Tagged<DescriptorArray>);Do we still need this? It feels like the remaining uses aren't necessary but I could easily miss something.
Great catch! I believe we no longer need any of the DescriptorArray specific barriers and we can remove all of them 😊
void MarkCompactCollector::WeakenStrongDescriptorArrays() {Do we still need strong descriptor arrays? Strictly speaking we shouldn't be in the deserializer when we trim DescriptorArrays because we don't trim with a stack. It's a bit indirect though but this is similar to what we do for strings. I guess could e.g. CHECK that none of the LocalHeaps is in the deserializer to make sure this holds. If so, not for this CL obviously.
We don't. The follow up CLs removes that type. That's what Michael was asking earlier to split to a new CL.
MarkValueLocal(descriptor_array);Omer Katz<3
Acknowledged
This barrier here shouldn't be necessary, right? The set_instance_descriptors above should mark it.
No, this one is not needed anymore. The write barrier is covered now by `set_instance_descriptors`.
Related details
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |