[heap] Make read-only MemoryChunk field conditionally optional [v8/v8 : main]
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Commit-Queue | +1 |
1 comment
Dominik: Overall
Leszek: Compiler parts
Related details
- Dominik Inführ
- Leszek Swirski
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11794339910000
Related details
- Dominik Inführ
- Leszek Swirski
- Michael Lippautz
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11590f5c510000
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
using IsReadOnlyPageField = IsSealedReadOnlySpaceField::Next<bool, 1>;Dominik: I finally spent this an explicit bit as there's various corner cases with the `owner()` tracking.
Lateset example: There's a state when metadata doesn't have an `owner_` anymore (after PreFree) and it looks like a RO page. We also call an accessor there that makes us fail a check in this case if we don't have an explicit bit.
Related details
- Dominik Inführ
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 4 comments![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
4 comments
// This check is precise and will cover all RO values and smal Smi values.Please fix this WARNING reported by Spellchecker: "smal" is a possible misspelling of "small".
To bypass Spellchecker, add a foot...
"smal" is a possible misspelling of "small".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
// This check is precise and will cover all RO values and smal Smi values.Please fix this WARNING reported by Spellchecker: "smal" is a possible misspelling of "small".
To bypass Spellchecker, add a foot...
"smal" is a possible misspelling of "small".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
#if V8_ENABLE_STICKY_MARK_BITS_BOOLmaybe out of scope for this CL, but this path is only for sticky mark bits, and is out-of-line (deferred) -- there seems to be no matching RO fast check in the inlined `kArchStoreWithWriteBarrier`, which is perhaps an oversight?
constexpr bool FastInReadOnlySpaceOrSmallSmi<V8HeapCompressionScheme>(this is actually compression scheme independent, right? At least for CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOL
Related details
- Dominik Inführ
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
3 comments
Thanks, LGTM
using IsReadOnlyPageField = IsSealedReadOnlySpaceField::Next<bool, 1>;Dominik: I finally spent this an explicit bit as there's various corner cases with the `owner()` tracking.
Lateset example: There's a state when metadata doesn't have an `owner_` anymore (after PreFree) and it looks like a RO page. We also call an accessor there that makes us fail a check in this case if we don't have an explicit bit.
Acknowledged
constexpr bool FastInReadOnlySpaceOrSmallSmi<V8HeapCompressionScheme>(this is actually compression scheme independent, right? At least for CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOL
In the function below we seem to DCHECK this as well. But I suppose now after your previous CL we should be able to do this for all 3 cages, right?
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
constexpr bool FastInReadOnlySpaceOrSmallSmi<V8HeapCompressionScheme>(Dominik Inführthis is actually compression scheme independent, right? At least for CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOL
In the function below we seem to DCHECK this as well. But I suppose now after your previous CL we should be able to do this for all 3 cages, right?
The idea here is the following:
- The function below (L493++) takes a `Tagged<MaybeObject>` and DCHECKs that `obj` is in the main cage.
- This function here takes only `Tagged_t` which could in theory come also from an `ExternalCodeCompressionScheme`. With the template parameter the caller guarantees that this is indeed a cage which is nicely 4G aligned and thus `<` works.
`Tagged_t` coming from `ExternalCodeCompressionScheme` would not work with the `<` check as the base is not 4G aligned.
Related details
- Dominik Inführ
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
constexpr bool FastInReadOnlySpaceOrSmallSmi<V8HeapCompressionScheme>(Dominik Inführthis is actually compression scheme independent, right? At least for CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOL
Michael LippautzIn the function below we seem to DCHECK this as well. But I suppose now after your previous CL we should be able to do this for all 3 cages, right?
The idea here is the following:
- The function below (L493++) takes a `Tagged<MaybeObject>` and DCHECKs that `obj` is in the main cage.
- This function here takes only `Tagged_t` which could in theory come also from an `ExternalCodeCompressionScheme`. With the template parameter the caller guarantees that this is indeed a cage which is nicely 4G aligned and thus `<` works.
`Tagged_t` coming from `ExternalCodeCompressionScheme` would not work with the `<` check as the base is not 4G aligned.
Right, but I thought that's the reason we also carved out the 8M red zone potentially in the middle of the code cage to prevent compressed values being less than 8M (and also main + trusted cages). Such that this comparison would be allowed on objects of all cages.
Related details
- Leszek Swirski
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 4 comments![]( "Open in Gerrit")
// This check is precise and will cover all RO values and smal Smi values.Please fix this WARNING reported by Spellchecker: "smal" is a possible misspelling of "small".
To bypass Spellchecker, add a foot...
"smal" is a possible misspelling of "small".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
// This check is precise and will cover all RO values and smal Smi values.Please fix this WARNING reported by Spellchecker: "smal" is a possible misspelling of "small".
To bypass Spellchecker, add a foot...
"smal" is a possible misspelling of "small".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
maybe out of scope for this CL, but this path is only for sticky mark bits, and is out-of-line (deferred) -- there seems to be no matching RO fast check in the inlined `kArchStoreWithWriteBarrier`, which is perhaps an oversight?
Yeah, let's do an independent change for that.
constexpr bool FastInReadOnlySpaceOrSmallSmi<V8HeapCompressionScheme>(Dominik Inführthis is actually compression scheme independent, right? At least for CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOL
Michael LippautzIn the function below we seem to DCHECK this as well. But I suppose now after your previous CL we should be able to do this for all 3 cages, right?
Dominik InführThe idea here is the following:
- The function below (L493++) takes a `Tagged<MaybeObject>` and DCHECKs that `obj` is in the main cage.
- This function here takes only `Tagged_t` which could in theory come also from an `ExternalCodeCompressionScheme`. With the template parameter the caller guarantees that this is indeed a cage which is nicely 4G aligned and thus `<` works.
`Tagged_t` coming from `ExternalCodeCompressionScheme` would not work with the `<` check as the base is not 4G aligned.
Right, but I thought that's the reason we also carved out the 8M red zone potentially in the middle of the code cage to prevent compressed values being less than 8M (and also main + trusted cages). Such that this comparison would be allowed on objects of all cages.
- Dominik Inführ
- Leszek Swirski
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
#if V8_ENABLE_STICKY_MARK_BITS_BOOLMichael Lippautzmaybe out of scope for this CL, but this path is only for sticky mark bits, and is out-of-line (deferred) -- there seems to be no matching RO fast check in the inlined `kArchStoreWithWriteBarrier`, which is perhaps an oversight?
Yeah, let's do an independent change for that.
Implemented this sparately and rebased on top.
Related details
- Dominik Inführ
- Leszek Swirski
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Commit-Queue | +1 |
2 comments
I think this can now land \o/ ptal again
#if V8_STATIC_ROOTS_BOOL && CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOLI am not sure why the flags were merged in here: https://chromium-review.googlesource.com/c/v8/v8/+/7210945
Related details
- Dominik Inführ
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
#if V8_STATIC_ROOTS_BOOL && CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOLI am not sure why the flags were merged in here: https://chromium-review.googlesource.com/c/v8/v8/+/7210945
Now that I think of this: Maybe it should be combined again because only contiguous RO space guaranteees that all the pages are indeed consecutive. Mhm
Related details
- Dominik Inführ
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
#if V8_STATIC_ROOTS_BOOL && CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOLMichael LippautzI am not sure why the flags were merged in here: https://chromium-review.googlesource.com/c/v8/v8/+/7210945
Now that I think of this: Maybe it should be combined again because only contiguous RO space guaranteees that all the pages are indeed consecutive. Mhm
Yeah that was the reason for my suggestion to merge the flags -- in our reality, they're merged anyway, and I asked cloudflare to enable CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE for their config so that we can consider it equivalent to V8_STATIC_ROOTS
Related details
- Dominik Inführ
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Commit-Queue | +1 |
1 comment
#if V8_STATIC_ROOTS_BOOL && CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOLMichael LippautzI am not sure why the flags were merged in here: https://chromium-review.googlesource.com/c/v8/v8/+/7210945
Leszek SwirskiNow that I think of this: Maybe it should be combined again because only contiguous RO space guaranteees that all the pages are indeed consecutive. Mhm
Yeah that was the reason for my suggestion to merge the flags -- in our reality, they're merged anyway, and I asked cloudflare to enable CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE for their config so that we can consider it equivalent to V8_STATIC_ROOTS
Thanks for confirming. I reverted these changes except for using the proper constant now for the bailouts.
Related details
- Dominik Inführ
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |