Skip to content
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

8234773: Fix ThreadsSMRSupport::_bootstrap_list #1921

Closed
wants to merge 3 commits into from

Conversation

kimbarrett
Copy link

@kimbarrett kimbarrett commented Jan 4, 2021

Please review this change to ThreadsList and the singleton _bootstrap_list.
ThreadsList is made noncopyable, with _bootstrap_list changed to be direct
initialized to avoid referencing the copy constructor. The ThreadsList
constructor now uses a static array for the 0-entry case, so that static
ctor/dtor of _bootstrap_list doesn't involve C-heap allocation.

Testing:
mach5 tier1

Local (linux-x64) build with -fno-elide-constructors and ran hotspot:tier1.
This has a failure unrelated to this change:
compiler/intrinsics/klass/CastNullCheckDroppingsTest.java fails with

Internal Error (../../src/hotspot/share/jfr/utilities/jfrVersionSystem.inline.hpp:98), pid=31086, tid=31713

assert(node->_live) failed: invariant

Filed new bug: https://bugs.openjdk.java.net/browse/JDK-8259036

/label hotspot-runtime
/summary Make ThreadsList noncopyable, direct initializing _bootstrap_list. Avoid C-heap allocation for _bootstrap_list.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1921/head:pull/1921
$ git checkout pull/1921

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 4, 2021

👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added rfr hotspot-runtime labels Jan 4, 2021
@openjdk
Copy link

openjdk bot commented Jan 4, 2021

@kimbarrett
The hotspot-runtime label was successfully added.

@openjdk
Copy link

openjdk bot commented Jan 4, 2021

@kimbarrett Setting summary to Make ThreadsList noncopyable, direct initializing _bootstrap_list. Avoid C-heap allocation for _bootstrap_list.

@mlbridge
Copy link

mlbridge bot commented Jan 4, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Kim,
Overall the fix looks good. I have one comment on the direct initialization part (after reading up on direct initialization).

Thanks,
David

@@ -76,7 +76,7 @@ volatile uint ThreadsSMRSupport::_deleted_thread_time_max = 0;
volatile uint ThreadsSMRSupport::_deleted_thread_times = 0;

// The bootstrap list is empty and cannot be freed.
ThreadsList ThreadsSMRSupport::_bootstrap_list = ThreadsList(0);
ThreadsList ThreadsSMRSupport::_bootstrap_list{0};
Copy link
Member

@dholmes-ora dholmes-ora Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To anyone unfamiliar with the details of direct initialization this is very obscure. I don't know how to interpret this given a ThreadsList has four member fields. Wouldn't {} have the same effect of implicitly zero-initializing all the fields?
Stylistically I find this syntax jarring, an equals sign in there would look far more natural to me. :(

@openjdk
Copy link

openjdk bot commented Jan 4, 2021

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8234773: Fix ThreadsSMRSupport::_bootstrap_list

Make ThreadsList noncopyable, direct initializing _bootstrap_list. Avoid C-heap allocation for _bootstrap_list.

Reviewed-by: dholmes, dcubed

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

  • 6f7723b: 8258792: LogCompilation: remove redundant check fixed by 8257518
  • 697bf7a: 8257740: Compiler crash when compiling type annotation on multicatch inside lambda

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jan 4, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 4, 2021

Mailing list message from Kim Barrett on hotspot-runtime-dev:

On Jan 3, 2021, at 9:16 PM, David Holmes <dholmes at openjdk.java.net> wrote:
[?]
Hi Kim,
Overall the fix looks good. I have one comment on the direct initialization part (after reading up on direct initialization).

Thanks,
David

src/hotspot/share/runtime/threadSMR.cpp line 79:

77:
78: // The bootstrap list is empty and cannot be freed.
79: ThreadsList ThreadsSMRSupport::_bootstrap_list{0};

To anyone unfamiliar with the details of direct initialization this is very obscure. I don't know how to interpret this given a ThreadsList has four member fields. Wouldn't `{}` have the same effect of implicitly zero-initializing all the fields?
Stylistically I find this syntax jarring, an equals sign in there would look far more natural to me. :(

ThreadsList is not an aggregate; it has a constructor - `ThreadsList(int)`.
The proposed change calls that 1-arg constructor with an argument of 0.

Are you saying you would prefer

ThreadsList ThreadsSMRSupport::_bootstrap_list = { 0 };

This is copy-list-initialization, again because `ThreadsList` is not an
aggregate. In this particular case, it would end up doing the same thing as
direct-initialization, i.e. call the 1-arg constructor.

However, if that constructor were `explicit`, the initialization is
ill-formed and compilation should fail. And that constructor ought to be
`explicit`. HotSpot code is unfortunately *really* bad about not using
`explicit` where it ought to be used though.

Since I'm touching code in this area and have been reminded of it, I'm
planning to change the constructor to be `explicit`.

Marked as reviewed by dholmes (Reviewer).

Thanks.

@mlbridge
Copy link

mlbridge bot commented Jan 4, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 4/01/2021 10:09 pm, Kim Barrett wrote:

On Jan 3, 2021, at 9:16 PM, David Holmes <dholmes at openjdk.java.net> wrote:
[?]
Hi Kim,
Overall the fix looks good. I have one comment on the direct initialization part (after reading up on direct initialization).

Thanks,
David

src/hotspot/share/runtime/threadSMR.cpp line 79:

77:
78: // The bootstrap list is empty and cannot be freed.
79: ThreadsList ThreadsSMRSupport::_bootstrap_list{0};

To anyone unfamiliar with the details of direct initialization this is very obscure. I don't know how to interpret this given a ThreadsList has four member fields. Wouldn't `{}` have the same effect of implicitly zero-initializing all the fields?
Stylistically I find this syntax jarring, an equals sign in there would look far more natural to me. :(

ThreadsList is not an aggregate; it has a constructor - `ThreadsList(int)`.
The proposed change calls that 1-arg constructor with an argument of 0.

Then I confess I do not understand enough about the nuances of C++
construction/copying semantics to know what this "direct initialization"
is actually about.

David
-----

Are you saying you would prefer

ThreadsList ThreadsSMRSupport::_bootstrap_list = { 0 };

This is copy-list-initialization, again because `ThreadsList` is not an
aggregate. In this particular case, it would end up doing the same thing as
direct-initialization, i.e. call the 1-arg constructor.

However, if that constructor were `explicit`, the initialization is
ill-formed and compilation should fail. And that constructor ought to be
`explicit`. HotSpot code is unfortunately *really* bad about not using
`explicit` where it ought to be used though.

Since I'm touching code in this area and have been reminded of it, I'm
planning to change the constructor to be `explicit`.

Marked as reviewed by dholmes (Reviewer).

Thanks.

@mlbridge
Copy link

mlbridge bot commented Jan 4, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Follow up ...

On 4/01/2021 11:08 pm, David Holmes wrote:

On 4/01/2021 10:09 pm, Kim Barrett wrote:

On Jan 3, 2021, at 9:16 PM, David Holmes <dholmes at openjdk.java.net>
wrote:
[?]
Hi Kim,
Overall the fix looks good. I have one comment on the direct
initialization part (after reading up on direct initialization).

Thanks,
David

src/hotspot/share/runtime/threadSMR.cpp line 79:

77:
78: // The bootstrap list is empty and cannot be freed.
79: ThreadsList ThreadsSMRSupport::_bootstrap_list{0};

To anyone unfamiliar with the details of direct initialization this
is very obscure. I don't know how to interpret this given a
ThreadsList has four member fields. Wouldn't `{}` have the same
effect of implicitly zero-initializing all the fields?
Stylistically I find this syntax jarring, an equals sign in there
would look far more natural to me. :(

ThreadsList is not an aggregate; it has a constructor -
`ThreadsList(int)`.
The proposed change calls that 1-arg constructor with an argument of 0.

Then I confess I do not understand enough about the nuances of C++
construction/copying semantics to know what this "direct initialization"
is actually about.

I should avoid emailing late in the evening :)

The use of the "{ 0 }" syntax confused me because in my reading of:

https://en.cppreference.com/w/cpp/language/direct_initialization

it seemed to me that that syntax was used for non-class types, but
apparently that is not the case.

Using:

ThreadsList ThreadsSMRSupport::_bootstrap_list(0);

as given in the bug report would have been much clearer to me as it more
obviously looks like a constructor invocation.

Are you saying you would prefer

ThreadsList ThreadsSMRSupport::_bootstrap_list = { 0 };

This is copy-list-initialization, again because `ThreadsList` is not an
aggregate. In this particular case, it would end up doing the same
thing as
direct-initialization, i.e. call the 1-arg constructor.

However, if that constructor were `explicit`, the initialization is
ill-formed and compilation should fail. And that constructor ought to be
`explicit`. HotSpot code is unfortunately *really* bad about not using
`explicit` where it ought to be used though.

Since I'm touching code in this area and have been reminded of it, I'm
planning to change the constructor to be `explicit`.

I am not familiar with use of "explicit" so can't comment on that aspect.

Thanks,
David
-----

Marked as reviewed by dholmes (Reviewer).

Thanks.

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up.
Like @dholmes-ora, I'm not familiar with this "explicit" stuff.

@dholmes-ora
Copy link
Member

dholmes-ora commented Jan 8, 2021

I'm now more familiar with the use of "explicit" so that part is fine too.

I'd prefer to use this syntax if it is applicable to this case:

ThreadsList ThreadsSMRSupport::_bootstrap_list(0);

as the { 0 } syntax just looks weird to me as an implicit constructor invocation. Your call.

Thanks,
David

@mlbridge
Copy link

mlbridge bot commented Jan 8, 2021

Mailing list message from Kim Barrett on hotspot-runtime-dev:

On Jan 8, 2021, at 8:15 AM, David Holmes <dholmes at openjdk.java.net> wrote:

On Tue, 5 Jan 2021 21:09:03 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:

Make ThreadsList constructor explicit.

Thumbs up.
Like @dholmes-ora, I'm not familiar with this "explicit" stuff.

I'm now more familiar with the use of "explicit" so that part is fine too.

Thanks.

I'd prefer to use this syntax if it is applicable to this case:

ThreadsList ThreadsSMRSupport::_bootstrap_list(0);

as the `{ 0 }` syntax just looks weird to me as an implicit constructor invocation. Your call.

Uniform initialization is new to us, so will take some getting used to. But
there are some real advantages, some of which are mentioned in JDK-8252588,
along with cppreference.

I don't think we should go back and change existing usage, but I think the
benefits make it worth consistently preferring going forward. I'm starting
to train my fingers to use the new form, though Ioi seems to be doing much
better at that than I am.

@mlbridge
Copy link

mlbridge bot commented Jan 8, 2021

Mailing list message from Kim Barrett on hotspot-runtime-dev:

On Jan 5, 2021, at 4:12 PM, Daniel D.Daugherty <dcubed at openjdk.java.net> wrote:

On Mon, 4 Jan 2021 12:19:14 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

Please review this change to ThreadsList and the singleton _bootstrap_list.
[?]

Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:

Make ThreadsList constructor explicit.

Thumbs up.
Like @dholmes-ora, I'm not familiar with this "explicit" stuff.

Thanks.

@kimbarrett
Copy link
Author

kimbarrett commented Jan 8, 2021

/integrate

@openjdk openjdk bot closed this Jan 8, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 8, 2021
@openjdk
Copy link

openjdk bot commented Jan 8, 2021

@kimbarrett Since your change was applied there have been 2 commits pushed to the master branch:

  • 6f7723b: 8258792: LogCompilation: remove redundant check fixed by 8257518
  • 697bf7a: 8257740: Compiler crash when compiling type annotation on multicatch inside lambda

Your commit was automatically rebased without conflicts.

Pushed as commit 10a6b0d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@kimbarrett kimbarrett deleted the bootstrap_thread_list branch Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
3 participants