[objects] Remove HeapObjectLayout and clean up Tagged pointer API [v8/v8 : main]
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Commit-Queue | +1 |
1 comment
Related details
- Jakob Linke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow added 17 comments![]( "Open in Gerrit")
if (!IsJSObject(self)) return false;Why is this line so pretty but line 5271 is not?
(Same for 5272, 5295, 5296.)
std::enable_if_t<std::is_base_of_v<HeapObject, HeapObjectSubtype>>> {This could be a `requires` clause. Also fine to clean that up separately.
DCHECK(IsJSProxy(Tagged<Object>(*prototype.object())) ||Why is this needed here but not in line 1151?
return m.Is(static_cast<int>(offsetof(HeapObject, map_)) - kHeapObjectTag);Why? `Int64Matcher::Is()` takes an `int64_t`.
Higher-level: Looks like the size of this CL could be reduced significantly with a global `int HeapObject::kMapOffset = 0` definition. In this particular case, I wouldn't even insist on using `offsetof` to compute it, because it's so exceedingly unlikely that we'd ever reshuffle the `map_` member of heap objects 😊
if (IsWasmObject(Tagged<Object>(current))) continue;Why is this needed here when line 498 doesn't need it?
static constexpr uint32_t kLengthOffset = sizeof(HeapObject);
static constexpr uint32_t kHeaderSize =
kLengthOffset + (TAGGED_SIZE_8_BYTES ? kTaggedSize : kApiInt32Size);
static_assert(sizeof(Super::Header) == kHeaderSize);These definitions are deprecated, right? (To be replaced by `offsetof`/`sizeof`.)
public:Isn't everything before this point `public` as well?
HeapObject* operator->() { return this; }
const HeapObject* operator->() const { return this; }I think we no longer need to override these, do we? Regular C++ should do the trick?
#include "v8-internal.h"This shouldn't come after the include that has to be the last include.
if (IsWasmObject(Tagged(this))) return roots.Object_string();Why here and not above (lines 572 - 586)?
CONDITIONAL_WRITE_BARRIER(Tagged<HeapObject>(this), kLastIndexOffset, value,Other places just pass `this` to `CONDITIONAL_WRITE_BARRIER`, I guess we could do that here too?
if (IsWasmObject(Tagged<Object>(receiver))) return false;Why? `receiver` already is a `Tagged<>`.
if (IsWasmObject(Tagged<Object>(*receiver))) return false;Doesn't `Handle::operator*` return a `Tagged<>`? Why the explicit constructor call?
#define ACCESSORS_NOCAGE(holder, name, type, offset) \V8_OBJECT class SmallOrderedHashTable : public HeapObjectLayout {Why drop this here, while most other class definitions are keeping it?
V8_OBJECT class V8_EXPORT_PRIVATE SwissNameDictionaryWhy drop this here, while most other class definitions are keeping it?
Related details
- Jakob Linke
- 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. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
- 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. |
Jakob Linke (Gerrit)
Jakob Linke voted and added 13 comments![]( "Open in Gerrit")
Votes added by Jakob Linke
| Code-Review | +1 |
13 comments
Just minor comments, lgtm otherwise
return m.Is(static_cast<int>(offsetof(HeapObject, map_)) - kHeapObjectTag);Why? `Int64Matcher::Is()` takes an `int64_t`.
Higher-level: Looks like the size of this CL could be reduced significantly with a global `int HeapObject::kMapOffset = 0` definition. In this particular case, I wouldn't even insist on using `offsetof` to compute it, because it's so exceedingly unlikely that we'd ever reshuffle the `map_` member of heap objects 😊
Plus there's the automated kFooOffset -> offsetof rewrite that I'm waiting to land. Your call, but I agree this CL would be nicer without offsetof AND sizeof (kHeaderSize) churn.
if ((IsJSObject(Tagged<Object>(*receiver)) &&Do you need `Tagged<..>()` here?
return reinterpret_cast<Address>(this) + offset;suggestion: `address() +`
inline bool Is##Name(HeapObject obj, PtrComprCageBase cage_base) { \Here, too
inline bool Is##Name(HeapObject obj) { \Stale by-value overload
class StructBodyDescriptor : public FlexibleBodyDescriptor<sizeof(HeapObject)> {No action needed here since many others do this too.. But it's a brittle pattern. It'd be much nicer to have a Super convention, similar.
// TODO(leszeks): Remove once we're using Tagged everywhere.Could we do this now? Passing T by value is now invalid, post-port
static_assert(std::is_base_of_v<HeapObject, U>);Is this U still needed? Could you inline `std::is_base_of_v<HeapObject, T>` instead?
// Implicit conversions and explicit casts to/from raw pointers.Stale comment block
V8_INLINE constexpr T& operator*() const { return *ToRawPtr(); }Are these actually `constexpr`?
Jakob Linke (Gerrit)
Jakob Linke added 1 comment![]( "Open in Gerrit")
template <typename To, typename From>These overloads are dead too
Leszek Swirski (Gerrit)
Leszek Swirski added 27 comments![]( "Open in Gerrit")
PTAL again, I ran a grep over the diff and cleaned up any unintentional `Tagged(foo)` or `Tagged<Foo>(foo)` construction.
if (!IsJSObject(self)) return false;Why is this line so pretty but line 5271 is not?
(Same for 5272, 5295, 5296.)
ugh, leftovers from incrementally trying to get things to pass while also changing IsFoo overloads and Tagged<T> constructors. I've cleaned this up.
DCHECK(IsJSProxy(Tagged<Object>(*prototype.object())) ||Why is this needed here but not in line 1151?
Done
return m.Is(static_cast<int>(offsetof(HeapObject, map_)) - kHeapObjectTag);Why? `Int64Matcher::Is()` takes an `int64_t`.
Higher-level: Looks like the size of this CL could be reduced significantly with a global `int HeapObject::kMapOffset = 0` definition. In this particular case, I wouldn't even insist on using `offsetof` to compute it, because it's so exceedingly unlikely that we'd ever reshuffle the `map_` member of heap objects 😊
it could also be a cast to `int64_t`, the key here was that `offsetof` was unsigned and the subtraction underflowed it.
I generally want to phase out the `kFooOffset` macros, where I've been inconsistently keeping around old ones in some cases where it made the CL too large, and immediately removing it in other cases. Maybe here the diff would have been cleaner without these changes, but I'd prefer not to revert them now that they're done.
If we were to keep it permanently for `map`, then I'd want a better idea of why we shouldn't then keep it for other fields.
if ((IsJSObject(Tagged<Object>(*receiver)) &&Do you need `Tagged<..>()` here?
Done
if (IsWasmObject(Tagged<Object>(current))) continue;Why is this needed here when line 498 doesn't need it?
Done
template <typename To, typename From>These overloads are dead too
Done
friend class HeapObject;Leszek SwirskiSelf-friend
Done
return reinterpret_cast<Address>(this) + offset;Leszek Swirskisuggestion: `address() +`
Done
public:Isn't everything before this point `public` as well?
Done
HeapObject* operator->() { return this; }
const HeapObject* operator->() const { return this; }I think we no longer need to override these, do we? Regular C++ should do the trick?
Done
#include "v8-internal.h"This shouldn't come after the include that has to be the last include.
funnily enough, this is a clangd error rather than a Gemini one.
if (IsWasmObject(Tagged(this))) return roots.Object_string();Why here and not above (lines 572 - 586)?
Done
CONDITIONAL_WRITE_BARRIER(Tagged<HeapObject>(this), kLastIndexOffset, value,Other places just pass `this` to `CONDITIONAL_WRITE_BARRIER`, I guess we could do that here too?
Done
if (IsWasmObject(Tagged<Object>(receiver))) return false;Why? `receiver` already is a `Tagged<>`.
Done
if (IsWasmObject(Tagged<Object>(*receiver))) return false;Doesn't `Handle::operator*` return a `Tagged<>`? Why the explicit constructor call?
Done
#define ACCESSORS_NOCAGE(holder, name, type, offset) \Rebasing note: crrev.com/c/7806587 has just dropped this macro.
Nice, rebased.
inline bool Is##Name(HeapObject obj, PtrComprCageBase cage_base) { \Leszek SwirskiHere, too
Done
inline bool Is##Name(HeapObject obj) { \Leszek SwirskiStale by-value overload
Done
class StructBodyDescriptor : public FlexibleBodyDescriptor<sizeof(HeapObject)> {No action needed here since many others do this too.. But it's a brittle pattern. It'd be much nicer to have a Super convention, similar.
I want to rethink BodyDescriptors entirely, so punting for now.
Why drop this here, while most other class definitions are keeping it?
bad rebase, thanks.
class V8_EXPORT_PRIVATE SwissNameDictionary : public HeapObject {Why drop this here, while most other class definitions are keeping it?
bad rebase
// TODO(leszeks): Remove once we're using Tagged everywhere.Could we do this now? Passing T by value is now invalid, post-port
Done
static_assert(std::is_base_of_v<HeapObject, U>);Is this U still needed? Could you inline `std::is_base_of_v<HeapObject, T>` instead?
I think we can just fully remove it actually.
template <typename U = T>Leszek SwirskiU is now unused
so it is, thanks
// Implicit conversions and explicit casts to/from raw pointers.Leszek SwirskiStale comment block
Done
V8_INLINE constexpr T& operator*() const { return *ToRawPtr(); }Are these actually `constexpr`?
Good point, they're not (I wish `reinterpret_cast` could be `constexpr` but that's the way it has to be, honestly not sure these were ever `constexpr` to start with)
Related details
- Jakob Kummerow
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Jakob Kummerow
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jakob Linke
| Code-Review | +1 |
2 comments
return m.Is(static_cast<int>(offsetof(HeapObject, map_)) - kHeapObjectTag);Leszek SwirskiWhy? `Int64Matcher::Is()` takes an `int64_t`.
Higher-level: Looks like the size of this CL could be reduced significantly with a global `int HeapObject::kMapOffset = 0` definition. In this particular case, I wouldn't even insist on using `offsetof` to compute it, because it's so exceedingly unlikely that we'd ever reshuffle the `map_` member of heap objects 😊
it could also be a cast to `int64_t`, the key here was that `offsetof` was unsigned and the subtraction underflowed it.
I generally want to phase out the `kFooOffset` macros, where I've been inconsistently keeping around old ones in some cases where it made the CL too large, and immediately removing it in other cases. Maybe here the diff would have been cleaner without these changes, but I'd prefer not to revert them now that they're done.
If we were to keep it permanently for `map`, then I'd want a better idea of why we shouldn't then keep it for other fields.
Yeah that's a valid q, and personally I don't see the `offsetof/sizeof` forms as a huge improvement over the kFooOffset macros. For `sizeof` specifically, there's also the matter of possible subtle differences between Foo::kHeaderSize, sizeof(Foo), and actual object size (eg with embedder fields and in-obj props).
I don't have strong opinions here, happy to go either way. Automating the `offsetof` transition works nicely. Just personally I'm not yet convinced it's better. Your thoughts?
Not a blocker for this cl.
#define OBJECT_CONSTRUCTORS(Type, ...)Is the plan to remove this in a followup?
Related details
- Jakob Kummerow
- 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. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
- Jakob Kummerow
- Jakob Linke
- 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 3 comments![]( "Open in Gerrit")
PTAL again, I updated docs which cleared the +1
return m.Is(static_cast<int>(offsetof(HeapObject, map_)) - kHeapObjectTag);Leszek SwirskiWhy? `Int64Matcher::Is()` takes an `int64_t`.
Higher-level: Looks like the size of this CL could be reduced significantly with a global `int HeapObject::kMapOffset = 0` definition. In this particular case, I wouldn't even insist on using `offsetof` to compute it, because it's so exceedingly unlikely that we'd ever reshuffle the `map_` member of heap objects 😊
Jakob Linkeit could also be a cast to `int64_t`, the key here was that `offsetof` was unsigned and the subtraction underflowed it.
I generally want to phase out the `kFooOffset` macros, where I've been inconsistently keeping around old ones in some cases where it made the CL too large, and immediately removing it in other cases. Maybe here the diff would have been cleaner without these changes, but I'd prefer not to revert them now that they're done.
If we were to keep it permanently for `map`, then I'd want a better idea of why we shouldn't then keep it for other fields.
Yeah that's a valid q, and personally I don't see the `offsetof/sizeof` forms as a huge improvement over the kFooOffset macros. For `sizeof` specifically, there's also the matter of possible subtle differences between Foo::kHeaderSize, sizeof(Foo), and actual object size (eg with embedder fields and in-obj props).
I don't have strong opinions here, happy to go either way. Automating the `offsetof` transition works nicely. Just personally I'm not yet convinced it's better. Your thoughts?
Not a blocker for this cl.
Well I'm hoping that at least `Foo::kHeaderSize` goes away in favour of `sizeof(Foo)` or explicit offsets of the end-of-header where appropriate. Actual object size already has conflicts between `kSize` and `SizeFor`, though admittedly variable-sized types didn't have a `kSize` and will have a `sizeof`. Really what I'd like to do in as many places as possible is remove offset-based access entirely and access fields directly by reference, obviously this doesn't apply to codegen though. Either way, since this is a reversible decision, let's leave it for a future discussion to help this land.
Is the plan to remove this in a followup?
It was, but you made me realise I could remove it here.
Related details
- Jakob Kummerow
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
2 comments
return m.Is(static_cast<int>(offsetof(HeapObject, map_)) - kHeapObjectTag);Leszek SwirskiWhy? `Int64Matcher::Is()` takes an `int64_t`.
Higher-level: Looks like the size of this CL could be reduced significantly with a global `int HeapObject::kMapOffset = 0` definition. In this particular case, I wouldn't even insist on using `offsetof` to compute it, because it's so exceedingly unlikely that we'd ever reshuffle the `map_` member of heap objects 😊
Jakob Linkeit could also be a cast to `int64_t`, the key here was that `offsetof` was unsigned and the subtraction underflowed it.
I generally want to phase out the `kFooOffset` macros, where I've been inconsistently keeping around old ones in some cases where it made the CL too large, and immediately removing it in other cases. Maybe here the diff would have been cleaner without these changes, but I'd prefer not to revert them now that they're done.
If we were to keep it permanently for `map`, then I'd want a better idea of why we shouldn't then keep it for other fields.
Yeah that's a valid q, and personally I don't see the `offsetof/sizeof` forms as a huge improvement over the kFooOffset macros. For `sizeof` specifically, there's also the matter of possible subtle differences between Foo::kHeaderSize, sizeof(Foo), and actual object size (eg with embedder fields and in-obj props).
I don't have strong opinions here, happy to go either way. Automating the `offsetof` transition works nicely. Just personally I'm not yet convinced it's better. Your thoughts?
Not a blocker for this cl.
Yeah, no need to block this CL.
I don't have strong feelings on `offsetof` vs `kFooOffset` in general. I see the following arguments:
- we're all used to `kFooOffset`, and it's less verbose, as a consequence I tend to find it more readable (but could probably get used to the alternative)
- `offsetof` is standard C++, not a V8ism
- defining `kFooOffset` for every field of every class adds boilerplate code
- `kHeaderSize` might need to stay in some cases, so arguably keeping the other constants too is consistent with that
So overall, `kFooOffset` doesn't bother me, but I'm not going to vocally oppose a switch to `offsetof`.
#include "v8-internal.h"Leszek SwirskiThis shouldn't come after the include that has to be the last include.
funnily enough, this is a clangd error rather than a Gemini one.
I configure my clangd with `-header-insertion=never` to avoid these stray edits.
Related details
- Jakob Linke
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke voted Code-Review+1![]( "Open in Gerrit")
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |