x86: Increase inline reference arraycopy threshold #20525
Conversation
|
@0xdaryl May I ask you to review this change? Thank you! @vijaysun-omr fyi |
This commit increases arraycopyRepMovsReferenceArrayThreshold from 32 bytes to 64 bytes or 128 bytes if AVX-512 is supported Signed-off-by: Annabelle Huo <[email protected]>
a7ehuo
force-pushed
the
system-arraycopy-perf-27-threshold-64-128
branch
from
fc41c4b to
f7dc1d8
Compare
a7ehuo
changed the title
| int32_t newThreshold = comp->getOptions()->getArraycopyRepMovsReferenceArrayThreshold(); | ||
|
|
||
| if ((repMovsThresholdBytes < newThreshold) && ((newThreshold == 64) || (newThreshold == 128))) | ||
| if ((newThreshold == 32) || (newThreshold == 64) || (newThreshold == 128)) |
There was a problem hiding this comment.
I remember being confused by this code when it was first delivered, and with the passage of time I am once again confused.
The documentation for the arraycopyRepMovsReferenceArrayThreshold option says that only 32, 64, or 128 bytes are supported (32 is the default). This means that this if branch should always be taken because the condition on L1905 will always be true. This also means that repMovsThresholdBytes will always be overwritten from what it was set to on L1902.
Is the purpose of the code on L1902 then just in case someone provided an unsupported value for arraycopyRepMovsReferenceArrayThreshold ? If so, I think the code could be written a bit differently to convey that. Specifically, move L1902 as an else to this if and add a comment saying that this path is for when an invalid reference array threshold is provided.
Or am I completely wrong in understanding this logic?
There was a problem hiding this comment.
Oh, maybe I have to view this from the perspective of your new OMR PR 7534 alongside this PR.
There was a problem hiding this comment.
Sorry my bad, I should have mentioned that this PR should be reviewed together with OMR PR #7534.
Without OMR PR #7534, this PR won't break the build, but it does depend on OMR PR #7534 to get the reference arraycopy threshold properly increased to 64/128.
In OMR PR #7534, the default values for all arraycopyRepMovs*ArrayThresholds are reset to 0 in OMROptions.hpp.
_arraycopyRepMovsByteArrayThreshold = 0;
_arraycopyRepMovsCharArrayThreshold = 0;
_arraycopyRepMovsIntArrayThreshold = 0;
_arraycopyRepMovsLongArrayThreshold = 0;
_arraycopyRepMovsReferenceArrayThreshold = 0;My intended logic is
(1) CG should set the threshold to the proper value: 64 or 128
(2) If someone sets a value in -Xjit options, the threshold will get updated only if the value is valid
|
Jenkins test sanity xlinux,xmac,win jdk21 depends eclipse-omr/omr#7534 |
Revert "Merge pull request #20525"
…R20525 (v0.49) Revert "Merge pull request #20525"
This commit increases arraycopyRepMovsReferenceArrayThreshold from 32 bytes to 64 bytes or 128 bytes if AVX-512 is supported