RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v9]
Fei Yang
fyang at openjdk.org
Tue Nov 5 00:26:54 UTC 2024
On Tue, 5 Nov 2024 00:18:17 GMT, Fei Yang <fyang at openjdk.org> wrote:
>>> Also, does this mean that the changes from 2 to frame::sender_sp_offset in all of the lines (267, 271 and 273) should be reverted?
>>>
>> I think the previous lines are okay because we are constructing the fp, whereas in here we want to read the old fp stored in this frame.
>
>> As the same code on aarch64 and x86-64 uses `frame::sender_sp_offset` I suggested to change the literal 2 into `frame::sender_sp_offset` in order to increase the readability, but I forgot that `frame::sender_sp_offset` is 0 on riscv64. However I do think it's a problem that several places throughout the code base uses a literal 2 when it should really be `frame::sender_sp_offset`. This type of code is very fiddly and I think we should do what we can to increase the readability, so maybe we need another `frame::XYZ` constant that is 2 for this case.
>
> Yeah, I was also considering this issue when we were porting loom. I guess maybe `frame::metadata_words` which equals 2. Since this is not the only place, I would suggest we do a separate cleanup PR.
>
>> Also, does this mean that the changes from 2 to `frame::sender_sp_offset` in all of the lines (267, 271 and 273) should be reverted?
>
> I agree with @pchilano in that we are fine with these places.
> Sorry, I also thought it matched the aarch64 one without checking. @RealFYang should I change it for `hf.sp() + frame::link_offset` or just leave it as it was?
Or maybe `hf.sp() - frame::metadata_words`. But since we have several other occurrences, I would suggest we leave it as it was and go with a separate PR for the cleanup.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1828566395
More information about the core-libs-dev
mailing list