[sparkplug+] Fix handler check in LoadField [v8/v8 : main]
Xu, Hao A (Gerrit)
Xu, Hao A added 1 comment![]( "Open in Gerrit")
Related details
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xu, Hao A (Gerrit)
Xu, Hao A added 1 comment![]( "Open in Gerrit")
I'm also wondering if we can start experiment to enable sparkplug-plus' flag to better trace the performance change? We have another CL that might need this flag to be opened: https://chromium-review.googlesource.com/c/v8/v8/+/7714255
Related details
- Leszek Swirski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/13336813090000
Related details
- Leszek Swirski
- Xu, Hao A
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 added 2 comments![]( "Open in Gerrit")
I'm surprised there's such a small performance impact if we were unconditionally missing on the indexed-field builtins, do you have a theory why this is?
DCHECK_EQ(kSmiShiftSize + kSmiTagSize, 1);`static_assert` (assuming that `if (SmiValuesAre31Bits())` can be `if constexpr`)
Related details
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xu, Hao A (Gerrit)
Xu, Hao A added 2 comments![]( "Open in Gerrit")
I'm surprised there's such a small performance impact if we were unconditionally missing on the indexed-field builtins, do you have a theory why this is?
I'm sorry if I confused you with the latest pinpoint test, in which I tried to evaluate the perf impact of avoiding sanitizing the smi receiver. The performance impact of fixing the indexed-field builtins should be 0.3% to Speedometer3:
https://pinpoint-dot-chromeperf.appspot.com/job/115360d3090000
https://pinpoint-dot-chromeperf.appspot.com/job/174e0f80890000
https://pinpoint-dot-chromeperf.appspot.com/job/117cd17f090000
`static_assert` (assuming that `if (SmiValuesAre31Bits())` can be `if constexpr`)
- 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. |
Igor Sheludko (Gerrit)
Igor Sheludko added 1 comment![]( "Open in Gerrit")
}How about just using `SmiAnd(var_handler.value(), SmiConstant(mask))` which does this optimization for you?
Related details
- Leszek Swirski
- Xu, Hao A
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 1 comment![]( "Open in Gerrit")
Xu, Hao AI'm surprised there's such a small performance impact if we were unconditionally missing on the indexed-field builtins, do you have a theory why this is?
I'm sorry if I confused you with the latest pinpoint test, in which I tried to evaluate the perf impact of avoiding sanitizing the smi receiver. The performance impact of fixing the indexed-field builtins should be 0.3% to Speedometer3:
https://pinpoint-dot-chromeperf.appspot.com/job/115360d3090000
https://pinpoint-dot-chromeperf.appspot.com/job/174e0f80890000
https://pinpoint-dot-chromeperf.appspot.com/job/117cd17f090000
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xu, Hao A (Gerrit)
Xu, Hao A added 1 comment![]( "Open in Gerrit")
How about just using `SmiAnd(var_handler.value(), SmiConstant(mask))` which does this optimization for you?
Actually `var_handler.value()` is not guaranteed to be a smi, so we should check the SmiTagBits as well.
Related details
- Igor Sheludko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 2 comments![]( "Open in Gerrit")
mask = (mask << (kSmiShiftSize + kSmiTagSize)) | kSmiTagMask;BTW, this would be 32 for 32-bit smi case and move the mask value out of the int range.
Xu, Hao AHow about just using `SmiAnd(var_handler.value(), SmiConstant(mask))` which does this optimization for you?
Actually `var_handler.value()` is not guaranteed to be a smi, so we should check the SmiTagBits as well.
Right, then probabaly SmiAnd is not going to simplify things here.
Related details
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xu, Hao A (Gerrit)
Xu, Hao A added 2 comments![]( "Open in Gerrit")
mask = (mask << (kSmiShiftSize + kSmiTagSize)) | kSmiTagMask;BTW, this would be 32 for 32-bit smi case and move the mask value out of the int range.
Done. Thanks!
Xu, Hao AHow about just using `SmiAnd(var_handler.value(), SmiConstant(mask))` which does this optimization for you?
Igor SheludkoActually `var_handler.value()` is not guaranteed to be a smi, so we should check the SmiTagBits as well.
Right, then probabaly SmiAnd is not going to simplify things here.
- Igor Sheludko
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 2 comments![]( "Open in Gerrit")
TruncateIntPtrToInt32(BitcastTaggedToWordForTagAndSmiBits(
SmiConstant(target_handler)))),`Int32Constant(static_cast<int32_t>(target_handler.ptr() & mask))`.
BitcastTaggedToWordForTagAndSmiBits(
SmiConstant(target_handler))),Ditto: `IntPtrConstant( .. & .. )`.
Related details
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 3 comments![]( "Open in Gerrit")
target_handler =While you are here: `Tagged<Smi> target_handler = `.
mask = (mask << (kSmiShiftSize + kSmiTagSize)) | kSmiTagMask;BTW, this way it might be simpler (smi construction is hidden in the function):
`Address mask = Smi::From31BitPattern(LoadHandler::KindBits::kMask | ...).ptr() | kSmiTagMask;`
But I'm leaving it up to you.
ReinterpretCast<Word32T>(var_handler.value());`TruncateIntPtrToInt32(BitcastTaggedToWordForTagAndSmiBits(..))`
Related details
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xu, Hao A (Gerrit)
Xu, Hao A added 5 comments![]( "Open in Gerrit")
While you are here: `Tagged<Smi> target_handler = `.
Done
mask = (mask << (kSmiShiftSize + kSmiTagSize)) | kSmiTagMask;BTW, this way it might be simpler (smi construction is hidden in the function):
`Address mask = Smi::From31BitPattern(LoadHandler::KindBits::kMask | ...).ptr() | kSmiTagMask;`But I'm leaving it up to you.
Done
ReinterpretCast<Word32T>(var_handler.value());Xu, Hao A`TruncateIntPtrToInt32(BitcastTaggedToWordForTagAndSmiBits(..))`
Done
TruncateIntPtrToInt32(BitcastTaggedToWordForTagAndSmiBits(
SmiConstant(target_handler)))),`Int32Constant(static_cast<int32_t>(target_handler.ptr() & mask))`.
Done
BitcastTaggedToWordForTagAndSmiBits(
SmiConstant(target_handler))),Ditto: `IntPtrConstant( .. & .. )`.
- Igor Sheludko
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko voted and added 2 comments![]( "Open in Gerrit")
Votes added by Igor Sheludko
| Code-Review | +1 |
2 comments
lgtm, thanks! what's the perf impact now?
Drive-by: avoid sanitizing smi receiver because we don't load its map
in LoadIC_Field.Nit: actually it does load the map, how about this:
`... because LoadIC_Field already handles smi receiver case.`
Related details
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xu, Hao A (Gerrit)
Xu, Hao A added 2 comments![]( "Open in Gerrit")
lgtm, thanks! what's the perf impact now?
This CL contributes a +0.3% improvement to speedometer3 on win11-perf bot when sparkplug-plus' flag is enabled. After this fix the landed sparkplug_plus CLs can contribute a +0.6% improvement:
https://pinpoint-dot-chromeperf.appspot.com/job/17e8cd27090000
We observed more improvement (>1%) locally after adjusting the PGO profile for sparkplug-plus. And some of our on-going CLs might have few regression without sparkplug-plus' flag enabled(https://chromium-review.googlesource.com/c/v8/v8/+/7699206). So we are also wondering whether it is a good time to start experimenting with enabling the flag before making more change.
Drive-by: avoid sanitizing smi receiver because we don't load its map
in LoadIC_Field.Nit: actually it does load the map, how about this:
`... because LoadIC_Field already handles smi receiver case.`
I'm not sure if I get your point but when receiver is a smi, the Builtin will directly goes to `Runtime_LoadIC_Miss_FromBaseline`. So in the Builtin we don't try to load the map from a smi receiver.
Related details
- Igor Sheludko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko added 2 comments![]( "Open in Gerrit")
Xu, Hao Algtm, thanks! what's the perf impact now?
This CL contributes a +0.3% improvement to speedometer3 on win11-perf bot when sparkplug-plus' flag is enabled. After this fix the landed sparkplug_plus CLs can contribute a +0.6% improvement:
https://pinpoint-dot-chromeperf.appspot.com/job/17e8cd27090000We observed more improvement (>1%) locally after adjusting the PGO profile for sparkplug-plus. And some of our on-going CLs might have few regression without sparkplug-plus' flag enabled(https://chromium-review.googlesource.com/c/v8/v8/+/7699206). So we are also wondering whether it is a good time to start experimenting with enabling the flag before making more change.
Nice! I guess you can try to enable it after 14.8 branch point, if it's stable enough.
Drive-by: avoid sanitizing smi receiver because we don't load its map
in LoadIC_Field.Xu, Hao ANit: actually it does load the map, how about this:
`... because LoadIC_Field already handles smi receiver case.`
I'm not sure if I get your point but when receiver is a smi, the Builtin will directly goes to `Runtime_LoadIC_Miss_FromBaseline`. So in the Builtin we don't try to load the map from a smi receiver.
That's exactly what I meant, the `AccessorAssembler::LoadIC_Field(..)` does handle smi receiver case by bailing out and thus the receiver sanitation on top of that is not necessary.
Related details
- Xu, Hao A
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xu, Hao A (Gerrit)
Xu, Hao A added 1 comment![]( "Open in Gerrit")
Drive-by: avoid sanitizing smi receiver because we don't load its map
in LoadIC_Field.Xu, Hao ANit: actually it does load the map, how about this:
`... because LoadIC_Field already handles smi receiver case.`
Igor SheludkoI'm not sure if I get your point but when receiver is a smi, the Builtin will directly goes to `Runtime_LoadIC_Miss_FromBaseline`. So in the Builtin we don't try to load the map from a smi receiver.
That's exactly what I meant, the `AccessorAssembler::LoadIC_Field(..)` does handle smi receiver case by bailing out and thus the receiver sanitation on top of that is not necessary.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xu, Hao A (Gerrit)
Xu, Hao A voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |