[objects] Extract object predicates and MapWord to separate header [v8/v8 : main]
0 views
Skip to first unread message
Leszek Swirski (Gerrit)
Apr 24, 2026, 4:09:49 AM (5 days ago) Apr 24
to Jakob Kummerow, Hannes Payer, Marco Vitale, devtools-...@chromium.org, dmercadi...@chromium.org, marja...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com, was...@google.com
Attention needed from Jakob Kummerow
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Commit-Queue | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Leszek Swirski . resolved
PTAL, AI managed to do a thing maybe
Related details
Attention is currently required from:
- Jakob Kummerow
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Apr 24, 2026, 10:14:13 AM (4 days ago) Apr 24
to android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, Jakob Kummerow, Hannes Payer, Marco Vitale, victorgo...@chromium.org, devtools-...@chromium.org, dmercadi...@chromium.org, marja...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com, was...@google.com
Attention needed from Jakob Kummerow
Message from Leszek Swirski![]( "Open in Gerrit")
bots are green, PTAL
Related details
Attention is currently required from:
- Jakob Kummerow
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Apr 27, 2026, 5:41:41 AM (yesterday) Apr 27
to Leszek Swirski, Jakob Kummerow, android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, Hannes Payer, Marco Vitale, victorgo...@chromium.org, devtools-...@chromium.org, dmercadi...@chromium.org, marja...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com, was...@google.com
Attention needed from Leszek Swirski
Jakob Kummerow voted and added 25 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
25 comments
Patchset-level comments
File-level comment, Patchset 9 (Latest):
Jakob Kummerow . resolved
Love it! LGTM with nits.
Next time please review the AI's work yourself before sending it out.
File src/compiler/js-call-reducer.cc
Line 49, Patchset 9 (Latest):
#include "src/wasm/wasm-objects.h"Jakob Kummerow . unresolved
Can you teach the AI to never include both `foo.h` and `foo-inl.h`?
File src/deoptimizer/translated-state.cc
Line 35, Patchset 9 (Latest):
#include "src/objects/string.h"Jakob Kummerow . unresolved
Unrelated: wait, line 33 said that line 34 had to be the last include?
How does that even work without breaking the build?!
File src/heap/heap-inl.h
File src/objects/map-word-inl.h
Line 8, Patchset 9 (Latest):
// clang-format offJakob Kummerow . unresolved
Why?
If the aim was to block automatic alpha-sorting, just add a comment:
```
// Include the non-inl header before the rest of the headers.
```
after the `map-word.h` line. (See e.g. `map-inl.h` for precedent.)
File src/objects/map-word.h
Line 11, Patchset 9 (Latest):
namespace v8 {
namespace internal {Jakob Kummerow . unresolved
If you want to be modern: `namespace v8::internal {` (and corresponding close at the end of the file)
Same in the other new files.
File src/objects/object-predicates-inl.h
File src/objects/object-predicates.h
Line 16, Patchset 9 (Latest):
class Object;
class HeapObject;
class Smi;
class Isolate;
class LocalIsolate;
class ReadOnlyRoots;
class EarlyReadOnlyRoots;Jakob Kummerow . unresolved
nit: alpha-sort please
File src/objects/objects-inl.h
Line 487, Patchset 9 (Latest):
// This should be in objects/map-inl.h, but can't, because of a cyclic
// dependency.Jakob Kummerow . unresolved
Orphaned (and hopefully outdated?) comment
Line 71, Patchset 9 (Latest):
#include "src/objects/map-word-inl.h"Jakob Kummerow . unresolved
No, this one does _not_ have to be the last include. Neither does line 73.
File src/objects/objects.h
Line 705, Patchset 9 (Latest):
// Heap objects typically have a map pointer in their first word. However,
// during GC other data (e.g. mark bits, forwarding addresses) is sometimes
// encoded in the first word. The class MapWord is an abstraction of the
// value in a heap object's first word.
//
// When external code space is enabled forwarding pointers are encoded asJakob Kummerow . unresolved
Orphaned comment. This has actually been moved, but hasn't been deleted here.
Line 679, Patchset 9 (Latest):
// For compatibility with TaggedImpl, and users of this header that don't pull
// in objects-inl.h
// TODO(leszeks): Remove once no longer needed.Jakob Kummerow . unresolved
Orphaned comment. Either move an adapted version to `object-predicates.h`, or drop it entirely.
File src/profiler/heap-snapshot-generator.cc
Line 32, Patchset 9 (Latest):
#include "src/objects/instance-type.h"Jakob Kummerow . unresolved
This can go now, replaced by the line above.
Same for lines 39, 45, 47.
File src/torque/implementation-visitor.cc
Line 4921, Patchset 9 (Latest):
factory_impl << "#include \"src/heap/factory-base.h\"\n";Jakob Kummerow . unresolved
This shouldn't be here, and neither should line 4923.
Line 4930, Patchset 9 (Latest):
factory_impl << "#include \"src/wasm/wasm-objects-inl.h\"\n";Jakob Kummerow . unresolved
Perhaps `wasm-objects-inl.h` should be added to `all-objects-inl.h` instead? That would also address the next two diff chunks.
File src/wasm/c-api.cc
Line 47, Patchset 9 (Latest):
#include "src/wasm/wasm-objects.h"Jakob Kummerow . unresolved
Replaced by the line below.
File src/wasm/wasm-objects-inl.h
Line 23, Patchset 9 (Latest):
#include "src/objects/heap-number.h"Jakob Kummerow . unresolved
Replaced by the line above.
File src/wasm/wasm-objects.cc
Line 7, Patchset 9 (Latest):
#include "src/objects/js-array-buffer-inl.h"Jakob Kummerow . unresolved
Before line 27 would be a good place for this.
File test/cctest/wasm/test-wasm-metrics.cc
Line 15, Patchset 9 (Latest):
#include "src/wasm/wasm-objects.h"Jakob Kummerow . unresolved
Subsumed by the line below.
File test/common/wasm/wasm-module-runner.cc
Line 16, Patchset 9 (Latest):
#include "src/wasm/wasm-objects.h"Jakob Kummerow . unresolved
Replaced by the line below.
Related details
Attention is currently required from:
- Leszek Swirski
Submit Requirements:
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)
Apr 27, 2026, 5:50:49 AM (yesterday) Apr 27
to Jakob Kummerow, android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, Hannes Payer, Marco Vitale, victorgo...@chromium.org, devtools-...@chromium.org, dmercadi...@chromium.org, marja...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com, was...@google.com
Leszek Swirski added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Jakob Kummerow . resolved
Love it! LGTM with nits.
Next time please review the AI's work yourself before sending it out.
Leszek Swirski
yeah sorry, I reviewed one version but not the version I'd sent out...
Related details
Attention set is empty
Submit Requirements:
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)
11:04 AM (9 hours ago) 11:04 AM
to Daniel Lehmann, Jakob Kummerow, android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, Hannes Payer, Marco Vitale, victorgo...@chromium.org, devtools-...@chromium.org, dmercadi...@chromium.org, marja...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com, was...@google.com
Attention needed from Jakob Kummerow
Leszek Swirski added 25 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 12 (Latest):
Leszek Swirski . resolved
ready for another round, this time artisinally pre-reviewed
File src/compiler/js-call-reducer.cc
Can you teach the AI to never include both `foo.h` and `foo-inl.h`?
Leszek Swirski
Done
File src/deoptimizer/translated-state.cc
Unrelated: wait, line 33 said that line 34 had to be the last include?
How does that even work without breaking the build?!
Leszek Swirski
Yeah no idea how this doens't break more often...
File src/objects/map-word-inl.h
Why?
If the aim was to block automatic alpha-sorting, just add a comment:
```
// Include the non-inl header before the rest of the headers.```
after the `map-word.h` line. (See e.g. `map-inl.h` for precedent.)
Leszek Swirski
Done
File src/objects/map-word.h
If you want to be modern: `namespace v8::internal {` (and corresponding close at the end of the file)
Same in the other new files.
Leszek Swirski
Nice, done.
File src/objects/object-predicates-inl.h
File src/objects/object-predicates.h
Line 16, Patchset 9:
class Object;
class HeapObject;
class Smi;
class Isolate;
class LocalIsolate;
class ReadOnlyRoots;
class EarlyReadOnlyRoots;Jakob Kummerow . resolved
Leszek Swirskinit: alpha-sort please
Done
File src/objects/objects-inl.h
Line 487, Patchset 9:
// This should be in objects/map-inl.h, but can't, because of a cyclic
// dependency.Jakob Kummerow . resolved
Orphaned (and hopefully outdated?) comment
Leszek Swirski
Done
Too many empty lines here
Leszek Swirski
Done
No, this one does _not_ have to be the last include. Neither does line 73.
Leszek Swirski
Done
File src/objects/objects.h
Line 705, Patchset 9:
// Heap objects typically have a map pointer in their first word. However,
// during GC other data (e.g. mark bits, forwarding addresses) is sometimes
// encoded in the first word. The class MapWord is an abstraction of the
// value in a heap object's first word.
//
// When external code space is enabled forwarding pointers are encoded asJakob Kummerow . resolved
Orphaned comment. This has actually been moved, but hasn't been deleted here.
Leszek Swirski
Done
Line 679, Patchset 9:
// For compatibility with TaggedImpl, and users of this header that don't pull
// in objects-inl.h
// TODO(leszeks): Remove once no longer needed.Jakob Kummerow . resolved
Orphaned comment. Either move an adapted version to `object-predicates.h`, or drop it entirely.
Leszek Swirski
Done
File src/profiler/heap-snapshot-generator.cc
This can go now, replaced by the line above.
Same for lines 39, 45, 47.
Leszek Swirski
Done
File src/torque/implementation-visitor.cc
Line 4921, Patchset 9:
factory_impl << "#include \"src/heap/factory-base.h\"\n";Jakob Kummerow . resolved
This shouldn't be here, and neither should line 4923.
Leszek Swirski
Done
Line 4930, Patchset 9:
factory_impl << "#include \"src/wasm/wasm-objects-inl.h\"\n";Jakob Kummerow . resolved
Perhaps `wasm-objects-inl.h` should be added to `all-objects-inl.h` instead? That would also address the next two diff chunks.
Leszek Swirski
I like it.
File src/wasm/c-api.cc
Replaced by the line below.
Leszek Swirski
Done
File src/wasm/wasm-objects-inl.h
Replaced by the line above.
Leszek Swirski
Done
File src/wasm/wasm-objects.cc
Before line 27 would be a good place for this.
Leszek Swirski
Done
File test/cctest/wasm/test-wasm-metrics.cc
Subsumed by the line below.
Leszek Swirski
Done
File test/common/wasm/wasm-module-runner.cc
Replaced by the line below.
Attention is currently required from:
- Jakob Kummerow
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
11:29 AM (8 hours ago) 11:29 AM
to Leszek Swirski, Jakob Kummerow, Daniel Lehmann, android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, Hannes Payer, Marco Vitale, victorgo...@chromium.org, devtools-...@chromium.org, dmercadi...@chromium.org, marja...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com, was...@google.com
Attention needed from Leszek Swirski
Jakob Kummerow voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Leszek Swirski
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
1:03 PM (7 hours ago) 1:03 PM
to Jakob Kummerow, Daniel Lehmann, android-bu...@system.gserviceaccount.com, v8-s...@luci-project-accounts.iam.gserviceaccount.com, Hannes Payer, Marco Vitale, victorgo...@chromium.org, devtools-...@chromium.org, dmercadi...@chromium.org, marja...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com, was...@google.com
Leszek Swirski voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
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)
1:05 PM (7 hours ago) 1:05 PM
to Leszek Swirski, Jakob Kummerow, Daniel Lehmann, android-bu...@system.gserviceaccount.com, Hannes Payer, Marco Vitale, victorgo...@chromium.org, devtools-...@chromium.org, dmercadi...@chromium.org, marja...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com, was...@google.com
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[objects] Extract object predicates and MapWord to separate header
Many headers include objects-inl.h because they want access to the IsFoo
object type predicates. We can extract these predicates out into a
smaller header, which can reduce the number of objects-inl.h includes
and hopefully let us resolve some include cycles.
Notably, I break the cycle of map-inl.h including objects-inl.h. I could
then also remove map-inl.h's reliance on wasm-objects-inl.h, fixing the
cascade of failures by including wasm-objects-inl.h elsewhere.
I also break the dependency of heap-inl.h needing objects-inl.h, by both
using the new object-predicates header, and splitting off MapWord into
its own header.
Change-Id: Id6823d95a140226ecb80dadd3f8778bd800da7c0
Reviewed-by: Jakob Kummerow <jkum...@chromium.org>
Commit-Queue: Leszek Swirski <les...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#106900}
Files:
- M BUILD.bazel
- M BUILD.gn
- A agents/skills/header-cycle-breaker/SKILL.md
- M src/asmjs/asm-js.cc
- M src/ast/prettyprinter.cc
- M src/builtins/builtins-callsite.cc
- M src/compiler/access-info.cc
- M src/compiler/js-call-reducer.cc
- M src/compiler/turbofan-graph-visualizer.cc
- M src/compiler/turboshaft/turbolev-graph-builder.cc
- M src/d8/d8.cc
- M src/debug/debug-interface.cc
- M src/debug/debug-stack-trace-iterator.cc
- M src/deoptimizer/deoptimizer.cc
- M src/deoptimizer/translated-state.cc
- M src/execution/isolate.cc
- M src/heap/heap-inl.h
- M src/objects/all-objects-inl.h
- M src/objects/backing-store.cc
- M src/objects/call-site-info.cc
- M src/objects/dictionary-inl.h
- M src/objects/feedback-cell-inl.h
- M src/objects/fixed-array.h
- M src/objects/heap-number-inl.h
- M src/objects/js-function.cc
- M src/objects/map-inl.h
- A src/objects/map-word-inl.h
- A src/objects/map-word.h
- M src/objects/module.cc
- M src/objects/name-inl.h
- A src/objects/object-predicates-inl.h
- A src/objects/object-predicates.h
- M src/objects/objects-inl.h
- M src/objects/objects.cc
- M src/objects/objects.h
- M src/objects/shared-function-info-inl.h
- M src/objects/shared-function-info.cc
- M src/profiler/heap-snapshot-generator.cc
- M src/runtime/runtime-test-wasm.cc
- M src/runtime/runtime-wasm.cc
- M src/torque/implementation-visitor.cc
- M src/wasm/c-api.cc
- M src/wasm/constant-expression-interface.cc
- M src/wasm/constant-expression.cc
- M src/wasm/interpreter/wasm-interpreter-runtime.cc
- M src/wasm/module-instantiate.cc
- M src/wasm/wasm-js.cc
- M src/wasm/wasm-module.cc
- M src/wasm/wasm-objects-inl.h
- M src/wasm/wasm-objects.cc
- M test/cctest/wasm/test-backing-store.cc
- M test/cctest/wasm/test-compilation-cache.cc
- M test/cctest/wasm/test-grow-memory.cc
- M test/cctest/wasm/test-wasm-metrics.cc
- M test/common/wasm/wasm-module-runner.cc
- M test/unittests/wasm/compilation-hints-unittest.cc
- M test/unittests/wasm/wasm-tracing-unittest.cc
Change size: XL
Delta: 57 files changed, 766 insertions(+), 592 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Jakob Kummerow
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages