[sandbox] Introduce ExternalBufferTable [v8/v8 : main]
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
Sorry, I haven't gotten around to reviewing this CL yet, it's been a pretty busy week (see https://v8.dev/blog/sandbox)... I'll try to do it first thing on Monday though!
Related details
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) added 1 comment![]( "Open in Gerrit")
Sorry, I haven't gotten around to reviewing this CL yet, it's been a pretty busy week (see https://v8.dev/blog/sandbox)... I'll try to do it first thing on Monday though!
Thank you for keeping track of this CL!
Nice high level overview, just read the blog post :)
Related details
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 8 comments![]( "Open in Gerrit")
Looks good overall, left some comments, and I think you need to rebase the CL
// See `ExternalPointerHandle` for the main documentation. The difference toNit: I feel like this block of code should move up, closer to ExternalPointer. Maybe into line 323, just above the
```
//
// External Pointers.
//
```
comment
// - If the slot is lazily-initialized, the handle may transition from theI wonder if we need to support lazy-initialization for external buffers. I think we don't, and so we could drop a bit of code, e.g. this comment here. Then the code below can probably be simplified to (but maybe it needs to be moved below ` if (handle == kNullExternalBufferHandle) return;` then)
```
#ifdef DEBUG
DCHECK_Eq(handle, base::AsAtomic32::Acquire_Load(
reinterpret_cast<ExternalBufferHandle*>(handle_location)));
#endif
```
uint32_t start_of_evacuation_area =
space->start_of_evacuation_area_.load(std::memory_order_relaxed);
if (V8_UNLIKELY(index >= start_of_evacuation_area)) {
space->AbortCompacting(start_of_evacuation_area);
}Can this code and the long comment above it move into the parent class (I guess some new helper function) to reduce duplication?
void ExternalBufferTableEntry::MakeExternalBufferEntry(I wonder if we want to have some sort of `DCHECK(IsExternalBufferPointerTag(tag))` here and introduce the respective predicate in v8-internal.h? We could then leave a comment there that, should we ever run out of external pointer tags, we could reuse tags for external buffers since they use a separate table.
inline std::pair<Address, size_t> Exchange(ExternalBufferHandle handle,Same here, I think this isn't currently needed
inline std::pair<Address, size_t> ExchangeExternalBuffer(I think this function is currently not needed, is it? If so, feel free to drop it to simplify things
uint32_t ExternalBufferTable::SweepAndCompact(Space* space,I'll think again about whether there's some way to reduce code duplication here. In the meantime, could you maybe just leave a TODO comment for that here?
Related details
- Deepak Mohan (Robo)
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) voted and added 7 comments![]( "Open in Gerrit")
Votes added by Deepak Mohan (Robo)
| Auto-Submit | +1 |
7 comments
// See `ExternalPointerHandle` for the main documentation. The difference toNit: I feel like this block of code should move up, closer to ExternalPointer. Maybe into line 323, just above the
```
//
// External Pointers.
//
```
comment
Acknowledged
// - If the slot is lazily-initialized, the handle may transition from theI wonder if we need to support lazy-initialization for external buffers. I think we don't, and so we could drop a bit of code, e.g. this comment here. Then the code below can probably be simplified to (but maybe it needs to be moved below ` if (handle == kNullExternalBufferHandle) return;` then)
```
#ifdef DEBUG
DCHECK_Eq(handle, base::AsAtomic32::Acquire_Load(
reinterpret_cast<ExternalBufferHandle*>(handle_location)));
#endif
```
Acknowledged
uint32_t start_of_evacuation_area =
space->start_of_evacuation_area_.load(std::memory_order_relaxed);
if (V8_UNLIKELY(index >= start_of_evacuation_area)) {
space->AbortCompacting(start_of_evacuation_area);
}Can this code and the long comment above it move into the parent class (I guess some new helper function) to reduce duplication?
Acknowledged
I wonder if we want to have some sort of `DCHECK(IsExternalBufferPointerTag(tag))` here and introduce the respective predicate in v8-internal.h? We could then leave a comment there that, should we ever run out of external pointer tags, we could reuse tags for external buffers since they use a separate table.
Acknowledged
inline std::pair<Address, size_t> Exchange(ExternalBufferHandle handle,Same here, I think this isn't currently needed
Acknowledged
inline std::pair<Address, size_t> ExchangeExternalBuffer(I think this function is currently not needed, is it? If so, feel free to drop it to simplify things
Acknowledged
uint32_t ExternalBufferTable::SweepAndCompact(Space* space,I'll think again about whether there's some way to reduce code duplication here. In the meantime, could you maybe just leave a TODO comment for that here?
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) added 1 comment![]( "Open in Gerrit")
return tag == kExternalStringResourceDataTag;Is this what you had in mind ? From the existing tags I can think of only this one tag that would use the buffer table. For external array buffer, we first want to introduce a compile time flag and a separate tag, so I decided to keep it out for separate CL.
Related details
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 8 comments![]( "Open in Gerrit")
Nice thanks! A couple more suggestions
return tag == kExternalStringResourceDataTag;Is this what you had in mind ? From the existing tags I can think of only this one tag that would use the buffer table. For external array buffer, we first want to introduce a compile time flag and a separate tag, so I decided to keep it out for separate CL.
Yep, exactly like this! Thanks!
With the new managed resources (see https://chromium-review.googlesource.com/c/v8/v8/+/5425025), could you add a `DCHECK(!IsManagedResourceTag(tag))` here since we don't expect them in the external buffer table and won't be able to handle them? Maybe also somewhere leave a comment to consider introducing a dedicated `ExternalBufferTag` enum which would guarantee that this can't happen.
using CompactibleSpace = CompactibleExternalEntityTable<How about
```
using super =
CompactibleExternalEntityTable<ExternalPointerTableEntry,
kExternalPointerTableReservationSize>;
```
Instead, like here: https://chromium-review.googlesource.com/c/v8/v8/+/5185345/18/src/sandbox/external-pointer-table.h
class V8_EXPORT_PRIVATE ExternalBufferTableCould you add a comment here briefly describing what this table is used for?
struct Payload {Nit: could you leave a TODO here like "TODO investigate whether we could have a reusable "TaggedPayload" class that can be used by the external pointer tables and the trusted pointer tables"?
inline void ExternalBufferMember<tag>::Init(IsolateForSandbox isolate,I wonder if these should move into a dedicated set of files (`external-buffer*`), wdyt?
AbortCompactingIfNeeded(space, index);Actually, I wonder: can this logic move into `AllocateEntry`, if `AllocateEntry` is added to `CompactibleExternalEntityTable`? It seems like we have all information available there, and I think that's also what's being done here: https://chromium-review.googlesource.com/c/v8/v8/+/5185345/18/src/sandbox/external-pointer-table.h
Related details
- Deepak Mohan (Robo)
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) voted and added 7 comments![]( "Open in Gerrit")
Votes added by Deepak Mohan (Robo)
| Auto-Submit | +1 |
7 comments
DCHECK(!IsManagedExternalPointerType(tag));will rebase again once https://chromium-review.googlesource.com/c/v8/v8/+/5439561 gets merged.
With the new managed resources (see https://chromium-review.googlesource.com/c/v8/v8/+/5425025), could you add a `DCHECK(!IsManagedResourceTag(tag))` here since we don't expect them in the external buffer table and won't be able to handle them? Maybe also somewhere leave a comment to consider introducing a dedicated `ExternalBufferTag` enum which would guarantee that this can't happen.
Acknowledged
using CompactibleSpace = CompactibleExternalEntityTable<How about
```
using super =
CompactibleExternalEntityTable<ExternalPointerTableEntry,
kExternalPointerTableReservationSize>;
```
Instead, like here: https://chromium-review.googlesource.com/c/v8/v8/+/5185345/18/src/sandbox/external-pointer-table.h
Acknowledged
Could you add a comment here briefly describing what this table is used for?
Acknowledged
Nit: could you leave a TODO here like "TODO investigate whether we could have a reusable "TaggedPayload" class that can be used by the external pointer tables and the trusted pointer tables"?
I can take a look at it in follow-up CL.
inline void ExternalBufferMember<tag>::Init(IsolateForSandbox isolate,I wonder if these should move into a dedicated set of files (`external-buffer*`), wdyt?
Makes sense, I guess that means we also need ExternalBuffer_t.
Actually, I wonder: can this logic move into `AllocateEntry`, if `AllocateEntry` is added to `CompactibleExternalEntityTable`? It seems like we have all information available there, and I think that's also what's being done here: https://chromium-review.googlesource.com/c/v8/v8/+/5185345/18/src/sandbox/external-pointer-table.h
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) added 1 comment![]( "Open in Gerrit")
std::atomic<Payload> payload_;Looking into the windows build failure https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket/8751140545198081025/+/u/build/compile/stdout?format=raw it comes from the atomic operation on 128-bit Payload type. I am thinking of removing the Payload wrapper and have the entry class as follows. Downside is we cannot explore a reusable TaggedPayload for the buffer table. Thoughts ?
```
struct ExternalBufferTableEntry {
private:
std::atomic<Address> encoded_word_;
std::atomic<size_t> raw_size_;
};
```
Related details
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 5 comments![]( "Open in Gerrit")
Nice, I like it! Please rebase once more and I can start another round of dry-runs
DCHECK(!IsManagedExternalPointerType(tag));will rebase again once https://chromium-review.googlesource.com/c/v8/v8/+/5439561 gets merged.
Acknowledged. It's landing just now, sorry for the delay/trouble with that Cl... :(
using super =Could you call this `Base` instead? Sorry, I realize that I suggested that name, but I've since looked through our code base and it seems we typically call this `Base`, and I've now done the same in the CompactibleExternalEntityTable class
std::atomic<Payload> payload_;Looking into the windows build failure https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket/8751140545198081025/+/u/build/compile/stdout?format=raw it comes from the atomic operation on 128-bit Payload type. I am thinking of removing the Payload wrapper and have the entry class as follows. Downside is we cannot explore a reusable TaggedPayload for the buffer table. Thoughts ?
```
struct ExternalBufferTableEntry {
private:
std::atomic<Address> encoded_word_;
std::atomic<size_t> raw_size_;
};
```
I think that's fine. That's basically what we already do for the code pointer table: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/sandbox/code-pointer-table.h;l=84;drc=0c86ce8471f83b0004a61a4d5fb7be29ac60e2cc
We could still use a "TaggedPayload" class since I guess we'd only need to tag one of the two values, the other can be a raw `Address` or `size_t`.
The only drawback is that the compiler seems unable to fold two atomic 64-bit loads into one atomic 128-bit load. Maybe worth keeping that as a comment, but I think it's definitely fine that way for now.
// If we evacuated all live entries in this segment then we can skip itWe've now removed this shortcut in https://chromium-review.googlesource.com/c/v8/v8/+/5439561 It should still be fine here since we don't support managed buffers (yet?), but maybe there's some value in keeping the implementations as close together as possible for now?
Related details
- Deepak Mohan (Robo)
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) voted and added 4 comments![]( "Open in Gerrit")
Votes added by Deepak Mohan (Robo)
| Auto-Submit | +1 |
Should be good for a dry-run, PTAL
Could you call this `Base` instead? Sorry, I realize that I suggested that name, but I've since looked through our code base and it seems we typically call this `Base`, and I've now done the same in the CompactibleExternalEntityTable class
Acknowledged
Samuel GroßLooking into the windows build failure https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket/8751140545198081025/+/u/build/compile/stdout?format=raw it comes from the atomic operation on 128-bit Payload type. I am thinking of removing the Payload wrapper and have the entry class as follows. Downside is we cannot explore a reusable TaggedPayload for the buffer table. Thoughts ?
```
struct ExternalBufferTableEntry {
private:
std::atomic<Address> encoded_word_;
std::atomic<size_t> raw_size_;
};
```
I think that's fine. That's basically what we already do for the code pointer table: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/sandbox/code-pointer-table.h;l=84;drc=0c86ce8471f83b0004a61a4d5fb7be29ac60e2cc
We could still use a "TaggedPayload" class since I guess we'd only need to tag one of the two values, the other can be a raw `Address` or `size_t`.The only drawback is that the compiler seems unable to fold two atomic 64-bit loads into one atomic 128-bit load. Maybe worth keeping that as a comment, but I think it's definitely fine that way for now.
Missed the codepointertable, thanks for the pointer! I see that it does not use Payload for the code_ object, during the TaggedPayload exploration should we try converting it as well ?
We could still use a "TaggedPayload" class since I guess we'd only need to tag one of the two values, the other can be a raw Address or size_t.
Oh yeah, sounds good.
The only drawback is that the compiler seems unable to fold two atomic 64-bit loads into one atomic 128-bit load.
Seems like this might be a clang only issue, on linux we can use gcc libatomic as fallback https://godbolt.org/z/4PbTYsvzW and windows msvc seems to support it natively.
// If we evacuated all live entries in this segment then we can skip itWe've now removed this shortcut in https://chromium-review.googlesource.com/c/v8/v8/+/5439561 It should still be fine here since we don't support managed buffers (yet?), but maybe there's some value in keeping the implementations as close together as possible for now?
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) voted and added 1 comment![]( "Open in Gerrit")
Votes added by Deepak Mohan (Robo)
| Auto-Submit | +1 |
Given this will be resolved in https://chromium-review.googlesource.com/c/v8/v8/+/5185345/comment/33849afa_14a4721e/, let me remove this to avoid conflict.
Related details
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
using Base =FWIW I use this definition in https://chromium-review.googlesource.com/c/v8/v8/+/5185345.
Related details
- Deepak Mohan (Robo)
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
using Base =FWIW I use this definition in https://chromium-review.googlesource.com/c/v8/v8/+/5185345.
Aah excuse me, I was misreading an interdiff. Please ignore :)
Related details
- Deepak Mohan (Robo)
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß voted and added 5 comments![]( "Open in Gerrit")
Votes added by Samuel Groß
| Code-Review | +1 |
| Commit-Queue | +1 |
5 comments
Nice, just a few more nits, otherwise LGTM from my side!
This change implements a pointer table where each entry is 16 BytesNit: 'Bytes' -> 'bytes'
#ifdef DEBUGNit: I think you can drop the `ifdef DEBUG` since the body is now a single `DCHECK`
// During compaction, entries that are evacuated may not be visited during
// sweeping and may therefore still have their marking bit set. As such, we
// should clear that here.
payload.ClearMarkBit();See my other comment above. This is no longer necessary now that we've removed the shortcut in SweepAndCompact
at(old_index).UnmarkAndMigrateInto(new_entry);Nit: with the simplification in `SweepAndCompact` this can simply become `MigrateInto` now and doesn't have to do anything with the marking bit anymore.
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) voted and added 4 comments![]( "Open in Gerrit")
Votes added by Deepak Mohan (Robo)
| Auto-Submit | +1 |
4 comments
This change implements a pointer table where each entry is 16 BytesDeepak Mohan (Robo)Nit: 'Bytes' -> 'bytes'
Acknowledged
#ifdef DEBUGNit: I think you can drop the `ifdef DEBUG` since the body is now a single `DCHECK`
Should this be a `CHECK` instead ?
// During compaction, entries that are evacuated may not be visited during
// sweeping and may therefore still have their marking bit set. As such, we
// should clear that here.
payload.ClearMarkBit();See my other comment above. This is no longer necessary now that we've removed the shortcut in SweepAndCompact
Acknowledged
Nit: with the simplification in `SweepAndCompact` this can simply become `MigrateInto` now and doesn't have to do anything with the marking bit anymore.
- Michael Lippautz
- Samuel Groß
Code-Review
| 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
| Code-Review | +1 |
2 comments
lgtm, only looked at API, exuction, and heap
constexpr size_t kExternalBufferTableReservationSize = 128 * MB;Introducing the CppPointerTable (which is just an EPT) caused OOMs for virtual memory on Android. The additional reservation may cause similar issues here.
Related details
- Deepak Mohan (Robo)
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Groß
| Code-Review | +1 |
1 comment
#ifdef DEBUGDeepak Mohan (Robo)Nit: I think you can drop the `ifdef DEBUG` since the body is now a single `DCHECK`
Should this be a `CHECK` instead ?
No I think a `DCHECK` is fine here
Related details
- Deepak Mohan (Robo)
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) voted and added 1 comment![]( "Open in Gerrit")
Votes added by Deepak Mohan (Robo)
| Auto-Submit | +1 |
1 comment
constexpr size_t kExternalBufferTableReservationSize = 128 * MB;Introducing the CppPointerTable (which is just an EPT) caused OOMs for virtual memory on Android. The additional reservation may cause similar issues here.
Thanks for bringing this up! I see the solution there was to half the reservation size.
@sa...@chromium.org should we have a fallback for the buffer table case where failure to allocate subspace should use regular EPT ? Or just half the reservation for android as done for EPT ?
Related details
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
constexpr size_t kExternalBufferTableReservationSize = 128 * MB;Deepak Mohan (Robo)Introducing the CppPointerTable (which is just an EPT) caused OOMs for virtual memory on Android. The additional reservation may cause similar issues here.
Thanks for bringing this up! I see the solution there was to half the reservation size.
@sa...@chromium.org should we have a fallback for the buffer table case where failure to allocate subspace should use regular EPT ? Or just half the reservation for android as done for EPT ?
I think a fallback case like that would add quite a bit of complexity and I'm not sure if it'd be secure, so I'd like to avoid that. I think we could try halving the size on Android (as we do for the other tables) and then see if that's good enough.
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) added 2 comments![]( "Open in Gerrit")
Thanks for the review!
constexpr size_t kExternalBufferTableReservationSize = 128 * MB;Deepak Mohan (Robo)Introducing the CppPointerTable (which is just an EPT) caused OOMs for virtual memory on Android. The additional reservation may cause similar issues here.
Samuel GroßThanks for bringing this up! I see the solution there was to half the reservation size.
@sa...@chromium.org should we have a fallback for the buffer table case where failure to allocate subspace should use regular EPT ? Or just half the reservation for android as done for EPT ?
I think a fallback case like that would add quite a bit of complexity and I'm not sure if it'd be secure, so I'd like to avoid that. I think we could try halving the size on Android (as we do for the other tables) and then see if that's good enough.
Sounds good. Addressed in https://chromium-review.googlesource.com/c/v8/v8/+/5456955
Related details
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 2 comments![]( "Open in Gerrit")
One more thing I just noticed
// This method is atomic and can be called from background threads.Now that we use two fields in the Entry struct, and set both in this function, I think this statement is no longer true. If we're unlucky, a `::Set` in one thread could interleave with a `::Get` in another thread and produce incorrect results. Maybe just update this comment to state that it is not atomic as it stores two fields?
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[sandbox] Introduce ExternalBufferTable
This change implements a pointer table where each entry is 16 bytes
of (external pointer, size) tuple. The table is intended for
accessing buffer data outside the sandbox in a memory safe way.
The table supports compaction and follows the same GC algorithm
as ExternalPointerTable.
- M BUILD.bazel
- M BUILD.gn
- M include/v8-internal.h
- M src/execution/isolate-data.h
- M src/execution/isolate.cc
- M src/execution/isolate.h
- M src/heap/heap.h
- M src/sandbox/compactible-external-entity-table.h
- A src/sandbox/external-buffer-inl.h
- A src/sandbox/external-buffer-table-inl.h
- A src/sandbox/external-buffer-table.cc
- A src/sandbox/external-buffer-table.h
- A src/sandbox/external-buffer.h
- M src/sandbox/external-pointer-inl.h
- M src/sandbox/isolate-inl.h
- M src/sandbox/isolate.h
Code-Review: +1 by Michael Lippautz, +1 by Samuel Groß
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) added 1 comment![]( "Open in Gerrit")
// This method is atomic and can be called from background threads.Now that we use two fields in the Entry struct, and set both in this function, I think this statement is no longer true. If we're unlucky, a `::Set` in one thread could interleave with a `::Get` in another thread and produce incorrect results. Maybe just update this comment to state that it is not atomic as it stores two fields?
Would it be okay if I make the change as part of https://chromium-review.googlesource.com/c/v8/v8/+/5406741 or do you prefer it in a separate CL ?
Also QQ, is there any situation were we would need to access the getter and setter methods from multiple threads today ?
Related details
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
// This method is atomic and can be called from background threads.Deepak Mohan (Robo)Now that we use two fields in the Entry struct, and set both in this function, I think this statement is no longer true. If we're unlucky, a `::Set` in one thread could interleave with a `::Get` in another thread and produce incorrect results. Maybe just update this comment to state that it is not atomic as it stores two fields?
Would it be okay if I make the change as part of https://chromium-review.googlesource.com/c/v8/v8/+/5406741 or do you prefer it in a separate CL ?
Also QQ, is there any situation were we would need to access the getter and setter methods from multiple threads today ?
Yeah that's fine! And yeah, I'm not sure this is currently needed at all (we probably only need AllocateAndInitialize), but I guess it's kind of awkward not to have a `Set` method? But I'm also fine with deleting the method if it's not needed anywhere
Related details
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |