[for-of-performance] Add IC slots to feedback vector [v8/v8 : main]
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 5 comments![]( "Open in Gerrit")
So the problem was fixed by your suggestion Leszek, thank you! I run the v8-perf test locally and added new numbers to the sheet we had. Unfortunately the numbers are not promising, adding the feedback slots slightly improved the performance but it does not look good enough. (sheet: https://docs.google.com/spreadsheets/d/1pdevwrKAyf3XXfQPuUVj6PNAOu_sijrns9CxxMZ5hk0/edit?gid=0#gid=0) I think we should discuss what the next steps would be in this situation.
Also, this CL is failing on the recent test you added in https://chromium-review.googlesource.com/c/v8/v8/+/7087027. I was not able to reproduce it locally to debug it, and I ran out of time for more debugging. I will take a look later if you wouldn't.
Thanks
TNode<UintPtrT> value_slot = UintPtrAdd(call_slot, UintPtrConstant(1));
TNode<UintPtrT> done_slot = UintPtrAdd(call_slot, UintPtrConstant(2));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.
Rezvan Mahdavi Hezavehthe 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`
Leszek SwirskiThat'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
```
Rezvan Mahdavi HezavehIt 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.
Thanks for finding the issue!
Branch(Word32Or(TaggedIsSmi(feedback_vector), IsUndefined(feedback_vector)),I realized that if feedback vector does not exist, it will be `undefined` or `smi`. Is that correct? Just making sure.
done_slot, feedback_vector);Rezvan Mahdavi Hezavehah this is the problem -- these should be passed in as `TaggedIndex` values, not `UintPtr`
Fixed, thanks!
feedback_spec()->AddLoadICSlot(); // value_slot
feedback_spec()->AddLoadICSlot(); // done_slotLeszek SwirskiMaybe these initializations are incorrect. Moving them to after the ForOfNext bytecode call did not make any difference.
Rezvan Mahdavi Hezavehthese should be correct
Acknowledged
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. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 1 comment![]( "Open in Gerrit")
So the problem was fixed by your suggestion Leszek, thank you! I run the v8-perf test locally and added new numbers to the sheet we had. Unfortunately the numbers are not promising, adding the feedback slots slightly improved the performance but it does not look good enough. (sheet: https://docs.google.com/spreadsheets/d/1pdevwrKAyf3XXfQPuUVj6PNAOu_sijrns9CxxMZ5hk0/edit?gid=0#gid=0) I think we should discuss what the next steps would be in this situation.
Also, this CL is failing on the recent test you added in https://chromium-review.googlesource.com/c/v8/v8/+/7087027. I was not able to reproduce it locally to debug it, and I ran out of time for more debugging. I will take a look later if you wouldn't.
Thanks
This one should not be marked as resolved.
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 added 2 comments![]( "Open in Gerrit")
Rezvan Mahdavi HezavehSo the problem was fixed by your suggestion Leszek, thank you! I run the v8-perf test locally and added new numbers to the sheet we had. Unfortunately the numbers are not promising, adding the feedback slots slightly improved the performance but it does not look good enough. (sheet: https://docs.google.com/spreadsheets/d/1pdevwrKAyf3XXfQPuUVj6PNAOu_sijrns9CxxMZ5hk0/edit?gid=0#gid=0) I think we should discuss what the next steps would be in this situation.
Also, this CL is failing on the recent test you added in https://chromium-review.googlesource.com/c/v8/v8/+/7087027. I was not able to reproduce it locally to debug it, and I ran out of time for more debugging. I will take a look later if you wouldn't.
Thanks
This one should not be marked as resolved.
Hm, that's a shame -- then the only thing I can think of is that it's missing inlining that is causing the difference. For me, running without the optimizing compilers gives much more similar results on the slow path, so this would imply the same thing.
Branch(Word32Or(TaggedIsSmi(feedback_vector), IsUndefined(feedback_vector)),Leszek SwirskiI realized that if feedback vector does not exist, it will be `undefined` or `smi`. Is that correct? Just making sure.
I thought it would only ever be undefined but there could be a bug somewhere passing Smi zero.
Related details
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |