[for-of-performance] Add feedback slot [v8/v8 : main]
0 views
Skip to first unread message
Rezvan Mahdavi Hezaveh (Gerrit)
Nov 12, 2025, 6:29:40 PM (7 days ago) Nov 12
to Leszek Swirski, AyeAye, dmercadi...@chromium.org, victorgo...@chromium.org, leszek...@chromium.org, verwaes...@chromium.org, v8-re...@googlegroups.com
Attention needed from Leszek Swirski
Rezvan Mahdavi Hezaveh added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 13 (Latest):
Rezvan Mahdavi Hezaveh . resolved
I feel like all the required changes have been made in the code to use feedback vectors. I added a slow path case to the existing tests. I am getting a crash on the turbofan, maglev, and baseline compilers (not ignition). All fail with the same error and stack trace
```
# Fatal error in ../../src/ic/ic.cc, line 2814
# Debug check failed: IsKeyedLoadICKind(kind).
#
#
#
#FailureMessage Object: 0x7be2916f9460
==== C stack trace ===============================
out/x64.debug/d8(__interceptor_backtrace+0x46) [0x5630c3a67816]
out/x64.debug/d8(v8::base::debug::StackTrace::StackTrace()+0x1e) [0x5630cf932bae]
out/x64.debug/d8(+0x16ea640b) [0x5630cf92640b]
out/x64.debug/d8(V8_Fatal(char const*, int, char const*, ...)+0x2e2) [0x5630cf8d7132]
out/x64.debug/d8(+0x16e56637) [0x5630cf8d6637]
out/x64.debug/d8(V8_Dcheck(char const*, int, char const*)+0x4d) [0x5630cf8d729d]
out/x64.debug/d8(+0xd23083f) [0x5630c5cb083f]
out/x64.debug/d8(v8::internal::Runtime_LoadIC_Miss(int, unsigned long*, v8::internal::Isolate*)+0x297) [0x5630c5caf567]
out/x64.debug/d8(+0x16258a7d) [0x5630cecd8a7d]
Trace/breakpoint trap
```
In gdb, I can see that in the second LoadIC call, kind is FeedbackSlotKind::kCall which is incorrect. I know having the same failure on all the optimization compilers means that the code inside code-stub-assembler has a problem and considering the gdb debug result, my guess is that the way I got the done_slot and value_slot indexes is incorrect (I added a comment on those lines). Do you have any suggestion on how to solve this problem?
File src/codegen/code-stub-assembler.cc
Line 17409, Patchset 13 (Latest):
TNode<UintPtrT> value_slot = UintPtrAdd(call_slot, UintPtrConstant(1));
TNode<UintPtrT> done_slot = UintPtrAdd(call_slot, UintPtrConstant(2));Rezvan Mahdavi Hezaveh . unresolved
I am pretty sure this part is the problem. My guess is that the slot indexes are not specified correctly here but I was not able to find any other way to do it.
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)
Nov 13, 2025, 6:55:55 AM (6 days ago) Nov 13
to Rezvan Mahdavi Hezaveh, AyeAye, dmercadi...@chromium.org, victorgo...@chromium.org, leszek...@chromium.org, verwaes...@chromium.org, v8-re...@googlegroups.com
Attention needed from Rezvan Mahdavi Hezaveh
Leszek Swirski added 1 comment![]( "Open in Gerrit")
File src/codegen/code-stub-assembler.cc
Line 17409, Patchset 13 (Latest):
TNode<UintPtrT> value_slot = UintPtrAdd(call_slot, UintPtrConstant(1));
TNode<UintPtrT> done_slot = UintPtrAdd(call_slot, UintPtrConstant(2));Rezvan Mahdavi Hezaveh . unresolved
I am pretty sure this part is the problem. My guess is that the slot indexes are not specified correctly here but I was not able to find any other way to do it.
Leszek Swirski
the problem you have here is that the call feedback (and load feedback) actually take up multiple slots. You can use `FeedbackMetadata::GetSlotSize(kind)` to get the right slot count to increment by, see also `FeedbackVectorSpec::AddSlot`
Related details
Attention is currently required from:
- Rezvan Mahdavi Hezaveh
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. |
Rezvan Mahdavi Hezaveh (Gerrit)
Nov 14, 2025, 4:36:25 PM (5 days ago) Nov 14
to Leszek Swirski, AyeAye, dmercadi...@chromium.org, victorgo...@chromium.org, leszek...@chromium.org, verwaes...@chromium.org, v8-re...@googlegroups.com
Attention needed from Leszek Swirski
Rezvan Mahdavi Hezaveh added 2 comments![]( "Open in Gerrit")
File src/codegen/code-stub-assembler.cc
Line 17409, Patchset 13:
TNode<UintPtrT> value_slot = UintPtrAdd(call_slot, UintPtrConstant(1));
TNode<UintPtrT> done_slot = UintPtrAdd(call_slot, UintPtrConstant(2));Rezvan Mahdavi Hezaveh . unresolved
Leszek SwirskiI am pretty sure this part is the problem. My guess is that the slot indexes are not specified correctly here but I was not able to find any other way to do it.
the problem you have here is that the call feedback (and load feedback) actually take up multiple slots. You can use `FeedbackMetadata::GetSlotSize(kind)` to get the right slot count to increment by, see also `FeedbackVectorSpec::AddSlot`
Rezvan Mahdavi Hezaveh
That's important! Thanks for the hint. Using the correct sizes resolved the previous issue. But the tests are still failing. When I printout the feedback vector, it looks like that some slots are uninitialized and some have incorrect content. I will paste an example here. Let's have our meeting next week and take a closer look at it.
The failure is:
```
# Fatal error in ../../src/objects/object-type.cc, line 87
# Type cast failed in CAST(Load<MaybeObject>( vector, IntPtrAdd(offset, IntPtrConstant(header_size)))) at ../../src/ic/accessor-assembler.cc:82
Expected HeapObjectReference but found Smi: 0x38 (56)
```
The call_slot index is 10, value_slot index is 12 and done_slot is 14. The feedback vector is:
```
feedback_vector: : DebugPrint: 0x7aba0082fab9: [FeedbackVector] in OldSpace
- map: 0x7aba00000871 <Map(FEEDBACK_VECTOR_TYPE)>
- length: 25
- shared function info: 0x7aba0082f7ad <SharedFunctionInfo testForOf>
- tiering_in_progress: 0
- osr_tiering_in_progress: 0
- invocation count: 6
- closure feedback cell array: 0x7aba000021b1: [ClosureFeedbackCellArray] in ReadOnlySpace
- map: 0x7aba00000849 <Map(CLOSURE_FEEDBACK_CELL_ARRAY_TYPE)>
- length: 0
- elements:
- slot #0 LoadGlobalNotInsideTypeof MONOMORPHIC
[weak] 0x7aba0082fa11 <PropertyCell name=0x7aba0082f6a1 <String[7]: #results> value=0x7aba00854215 <JSArray[0]>> {
[0]: [weak] 0x7aba0082fa11 <PropertyCell name=0x7aba0082f6a1 <String[7]: #results> value=0x7aba00854215 <JSArray[0]>>
[1]: 0x7aba00000e6d <Symbol: (uninitialized_symbol)>
}
- slot #2 SetNamedSloppy MONOMORPHIC
0x7aba0081bea9 <Map[16](PACKED_ELEMENTS)>: StoreHandler(Smi)(kind = kNativeDataProperty, descriptor = 0)
{
[2]: [weak] 0x7aba0081bea9 <Map[16](PACKED_ELEMENTS)>
[3]: 3
}
- slot #4 LoadProperty POLYMORPHIC
[weak] 0x7aba0081b75d <Map[16](PACKED_SMI_ELEMENTS)>: LoadHandler(do access check on lookup start object = 0, lookup on lookup start object = 0, kind = kConstantFromPrototype, data1 = [weak] 0x7aba0081baad <JSFunction values (sfi = 0x7aba00326019)>, validity cell = 0x7aba008148d1 <Cell value= [weak] 0x7aba00813761 <NativeContext[302]>>)
[weak] 0x7aba00822add <Map[24](HOLEY_ELEMENTS)>: LoadHandler(do access check on lookup start object = 0, lookup on lookup start object = 0, kind = kConstantFromPrototype, data1 = [weak] 0x7aba0081633d <JSFunction [Symbol.iterator] (sfi = 0x7aba00324ff9)>, validity cell = 0x7aba0082fb49 <Cell value= [weak] 0x7aba00813761 <NativeContext[302]>>)
[weak] 0x7aba0081c289 <Map[60](UINT8ELEMENTS)>: LoadHandler(do access check on lookup start object = 0, lookup on lookup start object = 0, kind = kConstantFromPrototype, data1 = [weak] 0x7aba00815679 <JSFunction values (sfi = 0x7aba0032a939)>, validity cell = 0x7aba0082fdc5 <Cell value= [weak] 0x7aba00813761 <NativeContext[302]>>) {
[4]: 0x7aba00854c3d <WeakFixedArray[6]>
[5]: 0x7aba00000e6d <Symbol: (uninitialized_symbol)>
}
- slot #6 Call POLYMORPHIC {
[6]: [weak] 0x7aba000060e1 <FeedbackCell[many closures]>
[7]: 56
}
- slot #8 LoadProperty MONOMORPHIC
[weak] 0x7aba00822add <Map[24](HOLEY_ELEMENTS)>: LoadHandler(do access check on lookup start object = 0, lookup on lookup start object = 0, kind = kConstantFromPrototype, data1 = [weak] 0x7aba00822b99 <JSFunction next (sfi = 0x7aba00326319)>, validity cell = 0x7aba0082fb49 <Cell value= [weak] 0x7aba00813761 <NativeContext[302]>>) {
[8]: [weak] 0x7aba00822add <Map[24](HOLEY_ELEMENTS)>
[9]: 0x7aba0082fb51 <Other heap object (LOAD_HANDLER_TYPE)>
}
- slot #10 Call MONOMORPHIC {
[10]: [weak] 0x7aba00822b99 <JSFunction next (sfi = 0x7aba00326319)>
[11]: 8
}
- slot #12 LoadProperty UNINITIALIZED {
[12]: 0x7aba00000e6d <Symbol: (uninitialized_symbol)>
[13]: 0x7aba00000e6d <Symbol: (uninitialized_symbol)>
}
- slot #14 LoadProperty UNINITIALIZED {
[14]: 0x7aba00000e6d <Symbol: (uninitialized_symbol)>
[15]: 0x7aba00000e6d <Symbol: (uninitialized_symbol)>
}
- slot #16 LoadProperty MONOMORPHIC
[weak] 0x7aba0081bea9 <Map[16](PACKED_ELEMENTS)>: LoadHandler(do access check on lookup start object = 0, lookup on lookup start object = 0, kind = kConstantFromPrototype, data1 = [weak] 0x7aba0081b95d <JSFunction push (sfi = 0x7aba00325dd9)>, validity cell = 0x7aba008148d1 <Cell value= [weak] 0x7aba00813761 <NativeContext[302]>>) {
[16]: [weak] 0x7aba0081bea9 <Map[16](PACKED_ELEMENTS)>
[17]: 0x7aba0082fb81 <Other heap object (LOAD_HANDLER_TYPE)>
}
- slot #18 Call MONOMORPHIC {
[18]: [weak] 0x7aba0081b95d <JSFunction push (sfi = 0x7aba00325dd9)>
[19]: 144
}
- slot #20 JumpLoop JumpLoop {
[20]: [cleared]
}
- slot #21 LoadProperty UNINITIALIZED {
[21]: 0x7aba00000e6d <Symbol: (uninitialized_symbol)>
[22]: 0x7aba00000e6d <Symbol: (uninitialized_symbol)>
}
- slot #23 Call UNINITIALIZED {
[23]: 0x7aba00000e6d <Symbol: (uninitialized_symbol)>
[24]: 0
}
0x7aba00000871: [Map] in ReadOnlySpace
- map: 0x7aba00000475 <MetaMap (0x7aba0000002d <null>)>
- type: FEEDBACK_VECTOR_TYPE
- instance size: variable
- elements kind: HOLEY_ELEMENTS
- enum length: invalid
- stable_map
- back pointer: 0x7aba00000011 <undefined>
- prototype_validity_cell: 0
- instance descriptors (own) #0: 0x7aba000007e5 <DescriptorArray[0]>
- prototype: 0x7aba0000002d <null>
- constructor: 0x7aba0000002d <null>
- dependent code: 0x7aba000007cd <Other heap object (WEAK_ARRAY_LIST_TYPE)>
- construction counter: 0
```
File src/interpreter/bytecode-generator.cc
Line 3380, Patchset 15 (Latest):
feedback_spec()->AddLoadICSlot(); // value_slot
feedback_spec()->AddLoadICSlot(); // done_slotRezvan Mahdavi Hezaveh . unresolved
Maybe these initializations are incorrect. Moving them to after the ForOfNext bytecode call did not make any difference.
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)
Nov 18, 2025, 8:23:13 AM (yesterday) Nov 18
to Rezvan Mahdavi Hezaveh, AyeAye, dmercadi...@chromium.org, victorgo...@chromium.org, leszek...@chromium.org, verwaes...@chromium.org, v8-re...@googlegroups.com
Attention needed from Rezvan Mahdavi Hezaveh
Leszek Swirski added 2 comments![]( "Open in Gerrit")
File src/codegen/code-stub-assembler.cc
Leszek Swirski
It looks like it's loading from slot 7 instead of slot 14 -- I suspect somewhere you have a uintptr slot index which is being treated like a smi, and getting divided by 2.
File src/interpreter/bytecode-generator.cc
Line 3380, Patchset 15 (Latest):
feedback_spec()->AddLoadICSlot(); // value_slot
feedback_spec()->AddLoadICSlot(); // done_slotRezvan Mahdavi Hezaveh . unresolved
Maybe these initializations are incorrect. Moving them to after the ForOfNext bytecode call did not make any difference.
Attention is currently required from:
- Rezvan Mahdavi Hezaveh
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)
Nov 18, 2025, 8:24:34 AM (yesterday) Nov 18
to Rezvan Mahdavi Hezaveh, AyeAye, dmercadi...@chromium.org, victorgo...@chromium.org, leszek...@chromium.org, verwaes...@chromium.org, v8-re...@googlegroups.com
Attention needed from Rezvan Mahdavi Hezaveh
Leszek Swirski added 1 comment![]( "Open in Gerrit")
File src/codegen/code-stub-assembler.cc
Line 17458, Patchset 15 (Latest):
done_slot, feedback_vector);Leszek Swirski . unresolved
ah this is the problem -- these should be passed in as `TaggedIndex` values, not `UintPtr`
Related details
Attention is currently required from:
- Rezvan Mahdavi Hezaveh
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. |
Reply all
Reply to author
Forward
0 new messages