[objects] Add a HashSeedWrapper for HashSeed data [v8/v8 : main]
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 2 comments![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Commit-Queue | +1 |
2 comments
#include "src/objects/fixed-array-inl.h"The mission: removing this include.
Related details
- Jakob Kummerow
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 9 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
9 comments
Love it! Just minor comments.
High-level: if I knew an expert for HeapObject layout definitions (😉), I'd ask them why `HashSeed`, `HashSeed::Data`, and `HashSeedWrapper` are three separate classes. In the old world, where one was a convenience view over a ByteArray storing the second, it kind of made sense, but in the new world, couldn't it be just _one_ class?
Cast<HashSeedWrapper>(this)->HashSeedWrapperVerify(isolate);Unrelated to this CL: a cleanup I've been wanting to do for a long time is deciding what "object verification" should really do. As of this CL, what we're implementing is:
```
switch (map()->instance_type()) {
case HASH_SEED_WRAPPER_TYPE:
CHECK(IsHashSeedWrapper(this)); // Instance type check.
break;
}
```
which isn't really useful (if it's a Foo, check that it's a Foo!). It's consistent with plenty of precedent though, so I guess we rather keep this than just not have an implementation at all...
template <AllocationType allocation = AllocationType::kYoung>I think this should be a regular function parameter (see e.g. line 164 for an example).
(We could even drop it entirely: AFAICS only the `AllocationType::kReadOnly` version is ever used. OTOH even if we currently don't have other users, having the parameter makes the behavior explicit.)
#include "src/objects/fixed-array-inl.h"The mission: removing this include.
\o/
Data* data = const_cast<Data*>(HashSeed(isolate).data_);I think if this was changed to `ReadOnlyRoots(isolate).hash_seed()->data()`, then the lines below could remain what they were. Was it an intentional choice to switch from writing into the object directly to accumulating an on-stack `Data` first before whole-sale memcopying it over? (I don't mind much either way.)
// Getters and setters for fields.nit: useless comment.
#include "src/heap/read-only-heap-inl.h"Not so sure about this; given how common `heap-object-inl.h` is I'd like to keep it as lean as possible.
Would it work to include `read-only-heap-inl.h` only in `string-inl.h`, and replace the two calls to `EarlyGetReadOnlyRoots()` in there with their inlined version `ReadOnlyHeap::EarlyGetReadOnlyRoots(this)`?
#include "src/objects/heap-object.h"Not needed if we have the `-inl.h` version.
(Interesting that we didn't need the latter here before. Probably came in transitively?)
Related details
- 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 voted and added 7 comments![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Commit-Queue | +1 |
7 comments
PTAL again (hopefully bots will be green)
template <AllocationType allocation = AllocationType::kYoung>I think this should be a regular function parameter (see e.g. line 164 for an example).
(We could even drop it entirely: AFAICS only the `AllocationType::kReadOnly` version is ever used. OTOH even if we currently don't have other users, having the parameter makes the behavior explicit.)
Done (stupid AI cargo culting)
I think if this was changed to `ReadOnlyRoots(isolate).hash_seed()->data()`, then the lines below could remain what they were. Was it an intentional choice to switch from writing into the object directly to accumulating an on-stack `Data` first before whole-sale memcopying it over? (I don't mind much either way.)
not intentional, a consequence of an intermediate state.
// Getters and setters for fields.Leszek Swirskinit: useless comment.
Done
#include "src/heap/read-only-heap-inl.h"Not so sure about this; given how common `heap-object-inl.h` is I'd like to keep it as lean as possible.
Would it work to include `read-only-heap-inl.h` only in `string-inl.h`, and replace the two calls to `EarlyGetReadOnlyRoots()` in there with their inlined version `ReadOnlyHeap::EarlyGetReadOnlyRoots(this)`?
Nice, done.
#include "src/objects/heap-object.h"Not needed if we have the `-inl.h` version.
(Interesting that we didn't need the latter here before. Probably came in transitively?)
Done
#include "src/sandbox/external-pointer.h"- Jakob Kummerow
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 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. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- 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 and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Auto-Submit | +1 |
1 comment
please restamp after spurious test change removed.
Related details
- Jakob Kummerow
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 voted![]( "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. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[objects] Add a HashSeedWrapper for HashSeed data
Refactor V8 string hash seed storage to use a newly introduced
HashSeedWrapper instead of a raw ByteArray in the read-only roots.
- Introduced HashSeedWrapper in roots.h and allocated it during heap
bootstrap.
- Wrapped the HashSeed::Data struct in UnalignedValueMember inside
HashSeedWrapper. It's unaligned relative to the struct alignment
because of the map pointer, but we ensure that it is allocated so that
the data itself is aligned by setting alignment dynamically based on
offsetof(HashSeedWrapper, data_).
- Restructured header dependencies to remove indirect objects-inl.h
inclusion from string-inl.h via
hash-seed-inl.h -> fixed-array-inl.h.
- This required moving CastTraits for Map and Symbol to
heap-object-inl.h.
- Also removed objects-inl.h from fixed-array-inl.h, though it still
includes it indirectly via factory-inl.h -- future work.
- Use ReadOnlyHeap::EarlyGetReadOnlyRoots directly instead of forcing
heap-object-inl.h to include it,
NO_IFTTT=Internal type, doesn't need compiler or JSObject handling.
TAG=agy
CONV=395db713-363f-4d55-9ebe-4fff13064bf3
- M BUILD.bazel
- M BUILD.gn
- M src/builtins/base.tq
- M src/codegen/code-stub-assembler.cc
- M src/compiler/heap-refs.h
- M src/deoptimizer/frame-translation-builder.cc
- M src/diagnostics/objects-debug.cc
- M src/diagnostics/objects-printer.cc
- M src/heap/factory.cc
- M src/heap/factory.h
- M src/heap/heap-visitor.h
- M src/heap/setup-heap-internal.cc
- M src/numbers/hash-seed-inl.h
- M src/numbers/hash-seed.cc
- M src/numbers/hash-seed.h
- M src/objects/all-objects-inl.h
- M src/objects/descriptor-array-inl.h
- M src/objects/fixed-array-inl.h
- A src/objects/hash-seed-wrapper-inl.h
- A src/objects/hash-seed-wrapper.h
- M src/objects/heap-object-inl.h
- M src/objects/heap-object.h
- M src/objects/map.cc
- M src/objects/map.h
- M src/objects/name-inl.h
- M src/objects/object-list-macros.h
- M src/objects/objects-body-descriptors-inl.h
- M src/objects/objects-inl.h
- M src/objects/objects.cc
- M src/objects/scope-info.cc
- M src/objects/shared-function-info-inl.h
- M src/objects/string-inl.h
- M src/objects/string.cc
- M src/objects/tagged-field.h
- M src/regexp/experimental/experimental-interpreter.cc
- M src/roots/roots.h
- M src/roots/static-roots-intl-nowasm.h
- M src/roots/static-roots-intl-wasm.h
- M src/roots/static-roots-nointl-nowasm.h
- M src/roots/static-roots-nointl-wasm.h
Code-Review: +1 by Jakob Kummerow
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |