-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8369238: Allow virtual thread preemption on some common class initialization paths #27802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
60cbca4
88844bf
d035366
79fce80
ac55715
9f4436a
30bdf49
6882db0
6000edb
a943c83
0e483ba
2786b1d
bea5620
1312486
3bf8ebd
4cecd7e
c7d6f5c
ffcd92a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,18 +28,17 @@ | |
| #include "runtime/frame.inline.hpp" | ||
| #include "runtime/registerMap.hpp" | ||
|
|
||
| class SmallRegisterMap; | ||
|
|
||
| // Java frames don't have callee saved registers (except for rfp), so we can use a smaller RegisterMap | ||
| class SmallRegisterMap { | ||
| constexpr SmallRegisterMap() = default; | ||
| ~SmallRegisterMap() = default; | ||
| NONCOPYABLE(SmallRegisterMap); | ||
| template <bool IncludeArgs> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a follow-on RFE to make SmallRegisterMap and it's new template in shared code. And only have the platform specific functions here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, good idea. |
||
| class SmallRegisterMapType { | ||
| friend SmallRegisterMap; | ||
|
|
||
| constexpr SmallRegisterMapType() = default; | ||
| ~SmallRegisterMapType() = default; | ||
| NONCOPYABLE(SmallRegisterMapType); | ||
|
|
||
| public: | ||
| static const SmallRegisterMap* instance() { | ||
| static constexpr SmallRegisterMap the_instance{}; | ||
| return &the_instance; | ||
| } | ||
| private: | ||
| static void assert_is_rfp(VMReg r) NOT_DEBUG_RETURN | ||
| DEBUG_ONLY({ assert (r == rfp->as_VMReg() || r == rfp->as_VMReg()->next(), "Reg: %s", r->name()); }) | ||
| public: | ||
|
|
@@ -71,7 +70,7 @@ class SmallRegisterMap { | |
|
|
||
| bool update_map() const { return false; } | ||
| bool walk_cont() const { return false; } | ||
| bool include_argument_oops() const { return false; } | ||
| bool include_argument_oops() const { return IncludeArgs; } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You made this a template rather than having an _include_argument_oops property for performance? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, using a bool member would work too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, good to know. We could or might not change it in the follow-up issue to move SmallRegisterMap to common code. |
||
| void set_include_argument_oops(bool f) {} | ||
| bool in_cont() const { return false; } | ||
| stackChunkHandle stack_chunk() const { return stackChunkHandle(); } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,17 +108,14 @@ inline int StackChunkFrameStream<frame_kind>::interpreter_frame_stack_argsize() | |
| } | ||
|
|
||
| template <ChunkFrames frame_kind> | ||
| inline int StackChunkFrameStream<frame_kind>::interpreter_frame_num_oops() const { | ||
| template <typename RegisterMapT> | ||
| inline int StackChunkFrameStream<frame_kind>::interpreter_frame_num_oops(RegisterMapT* map) const { | ||
| assert_is_interpreted_and_frame_type_mixed(); | ||
| ResourceMark rm; | ||
| InterpreterOopMap mask; | ||
| frame f = to_frame(); | ||
| f.interpreted_frame_oop_map(&mask); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two uses of this function left in continuationHelper.inline.hpp and continuationFreezeThaw.cpp under verification code. Maybe they can be removed? Do the places that call this in verification code still valid for preempted class initialization? Do they need to count arguments now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The verification code in |
||
| return mask.num_oops() | ||
| + 1 // for the mirror oop | ||
| + (f.interpreter_frame_method()->is_native() ? 1 : 0) // temp oop slot | ||
| + pointer_delta_as_int((intptr_t*)f.interpreter_frame_monitor_begin(), | ||
| (intptr_t*)f.interpreter_frame_monitor_end())/BasicObjectLock::size(); | ||
| InterpreterOopCount closure; | ||
| f.oops_interpreted_do(&closure, map); | ||
| return closure.count(); | ||
| } | ||
|
|
||
| template<> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
push_cleanup_continuation sets sp[-2]. This doesn't have to set that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
push_cleanup_continuation()doesn’t need it. I removed it there and added a comment on both methods.