Introduce Relaxed_FetchOr for MarkBit::Set [v8/v8 : main]
Hao A Xu (Gerrit)
Hao A Xu added 1 comment![]( "Open in Gerrit")
Related details
- Michael Lippautz
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 mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/124bab21510000
Related details
- Hao A Xu
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
1 comment
lgtm
The bots may show a possible regression though; maybe we need to revert this. It's unclear to me where that would come frome given on the ABI this should be better or the same as before. The CAS loop also has a filtering bailout.
Related details
- Dominik Inführ
- Hao A Xu
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hao A Xu (Gerrit)
Hao A Xu added 1 comment![]( "Open in Gerrit")
lgtm
The bots may show a possible regression though; maybe we need to revert this. It's unclear to me where that would come frome given on the ABI this should be better or the same as before. The CAS loop also has a filtering bailout.
Are the bots tested with PGO? I'm not sure if there will be less functions inlined if we made a change like this. It seems to be less likely that an actual data race will happen during the CAS, so I'm not sure whether the filtering bailout works many times.
For x64, the assembly behind FetchOr should be a single atomic bitwise or instruction with a LOCK prefix. I'm not a arm expert but it seems that they have a similar `LDSET` instruction as well. Maybe we can consider making this architecture dependent if it does incur regression on arm? (seems that win-11 bots have some improvement)
Related details
- Dominik Inführ
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
Hao A Xulgtm
The bots may show a possible regression though; maybe we need to revert this. It's unclear to me where that would come frome given on the ABI this should be better or the same as before. The CAS loop also has a filtering bailout.
Are the bots tested with PGO? I'm not sure if there will be less functions inlined if we made a change like this. It seems to be less likely that an actual data race will happen during the CAS, so I'm not sure whether the filtering bailout works many times.
For x64, the assembly behind FetchOr should be a single atomic bitwise or instruction with a LOCK prefix. I'm not a arm expert but it seems that they have a similar `LDSET` instruction as well. Maybe we can consider making this architecture dependent if it does incur regression on arm? (seems that win-11 bots have some improvement)
Yeah, I think it's just okay to land and try and let PGO pick it up.
Really, from what I understand this should always be better or the same.
Related details
- Dominik Inführ
- Hao A Xu
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hao A Xu (Gerrit)
Hao A Xu added 1 comment![]( "Open in Gerrit")
Hao A Xulgtm
The bots may show a possible regression though; maybe we need to revert this. It's unclear to me where that would come frome given on the ABI this should be better or the same as before. The CAS loop also has a filtering bailout.
Michael LippautzAre the bots tested with PGO? I'm not sure if there will be less functions inlined if we made a change like this. It seems to be less likely that an actual data race will happen during the CAS, so I'm not sure whether the filtering bailout works many times.
For x64, the assembly behind FetchOr should be a single atomic bitwise or instruction with a LOCK prefix. I'm not a arm expert but it seems that they have a similar `LDSET` instruction as well. Maybe we can consider making this architecture dependent if it does incur regression on arm? (seems that win-11 bots have some improvement)
Yeah, I think it's just okay to land and try and let PGO pick it up.
Really, from what I understand this should always be better or the same.
Thanks, I will land this CL.
I think in theory this should always be better or the same as well, I'm just saying that making it architecture dependent can give us some time to investigate where the regression on arm comes from.
Related details
- Dominik Inführ
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hao A Xu (Gerrit)
Hao A Xu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Dominik Inführ
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Introduce Relaxed_FetchOr for MarkBit::Set
Relaxed_SetBits were using Relaxed_CompareAndSwap to set the bits. It
contains a loop to ensure that the three operations (getting bits
/bitwise or/setting bits) are atomic. However, when all the bits to be
set are 1, we can use atomic fetch_or directly to avoid this loop.
This can be used in MarkBit::Set.
- M src/base/atomic-utils.h
- M src/base/atomicops.h
- M src/heap/marking-inl.h
- M src/heap/marking.h
- M test/unittests/base/atomic-utils-unittest.cc
Code-Review: +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |