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

8253717: Relocate stack overflow code out of thread.hpp/cpp #522

Closed
wants to merge 7 commits into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Oct 6, 2020

This change moves the significant amount of stack overflow related code (with ascii art!) out of thread files into a new file. Many of the functions are static functions and some go through JavaThread::_stack_overflow_state where needed. All functions are moved and not modified except for qualification.

I also added a delegating constructor to JavaThread::JavaThread so reordered the assignments as initializers from JavaThread::initialize.

Tested with tier1-6 and builds on arm32, ppc, s390 and zero.


Progress

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

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (3/3 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) (1/9 failed) ✔️ (9/9 passed)

Failed test task

Issue

  • JDK-8253717: Relocate stack overflow code out of thread.hpp/cpp

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 6, 2020

👋 Welcome back coleenp! 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 the rfr Pull request is ready for review label Oct 6, 2020
@openjdk
Copy link

openjdk bot commented Oct 6, 2020

@coleenp The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 6, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 6, 2020

Webrevs

robehn
robehn approved these changes Oct 6, 2020
Copy link
Contributor

@robehn robehn left a comment

Choose a reason for hiding this comment

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

Thanks

@openjdk
Copy link

openjdk bot commented Oct 6, 2020

@coleenp 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:

8253717: Relocate stack overflow code out of thread.hpp/cpp

Reviewed-by: rehn, dcubed, dholmes, stuefe

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 32 new commits pushed to the master branch:

  • abe2593: 8232092: (fs) Files::isWritable returns false on a writeable root directory (win)
  • 5a9bd41: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests
  • 38159d5: 8253876: jdk/test/lib/hexdump/ASN1FormatterTest.java fails with "AssertionError: Lines expected [126] but found [202]"
  • db3053d: 8067127: Tags cleanup
  • 739347f: 8254168: Remove TemplateTable::count_calls
  • 4fe07cc: 8252324: Signal related code should be shared among POSIX platforms
  • 1e8e543: 8216497: javadoc should auto-link to platform classes
  • 04ca660: 8253874: [JVMCI] added test omitted in 8252881
  • 49128a1: 8253475: Javadoc clean up in HttpExchange and HttpServer
  • 9543d76: 8253000: Remove redundant MAKE_SUBDIR argument
  • ... and 22 more: https://git.openjdk.java.net/jdk/compare/5d84e95ed591692818c189e640d8884b4ebd8431...master

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 Pull request is ready to be integrated label Oct 6, 2020
Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Really nice refactoring and cleanup. Thumbs up.

Any idea whether the change in compilation unit will have any performance effects?

src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/thread.cpp Outdated Show resolved Hide resolved
@coleenp
Copy link
Contributor Author

coleenp commented Oct 6, 2020

To answer the performance question, since it's not an actual indirection, the compiler should be smart enough to adjust the offset of the field to reflect its offset in stack_overflow_state. So this wouldn't make a difference in generated code. Even if it did, there are few places where stack overflow is accessed via thread and these are generally during exception handling. The stack overflow checking code mostly uses the static shadow, yellow and red sizes.

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Thumbs up!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Coleen,

The code reorganisation seems okay though I'm not clear on the motivation as stackoverflow protection is a feature of JavaThreads. I have a couple of minor comments.

Thanks,
David

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and feedback on other queries!

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

Hi Coleen,

this is a nice cleanup. Not a full review yet.

Also some of my remarks are probably follow ups (if that), I leave that up to you how much you want to take from my review.

Cheers, Thomas

src/hotspot/share/runtime/stackOverflow.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/stackOverflow.hpp Outdated Show resolved Hide resolved
@tstuefe
Copy link
Member

tstuefe commented Oct 7, 2020

Github seems to order code comments by time, not by line number; this is not ideal :(

tstuefe
tstuefe approved these changes Oct 7, 2020
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

Thanks Coleen for the cleanup and for taking my input. This looks good to me now.

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Based on what you wrote here in the invite:

All functions are moved and not modified except for qualification.
I also added a delegating constructor to JavaThread::JavaThread so reordered the assignments as initializers from JavaThread::initialize.

I didn't do my usual crawl through review of the old code and the new
code to make sure things "were the same". However, based on comments
posted by other folks there have been changes to the code after
movement, e.g., the change from NULL to nullptr. I'm going to
have to assume that other folks did a crawl through reviews.

Thumbs up.

src/hotspot/share/runtime/stackOverflow.hpp Outdated Show resolved Hide resolved
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

One small nit, otherwise okay.

src/hotspot/share/runtime/stackOverflow.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

With regard to the discussion on whether "*_base()" functions are inclusive or exclusive, the stack_base() function is exclusive and we fixed a number of checks that were incorrectly checking <= stack_base() rather than < stack_base(). See:
https://bugs.openjdk.java.net/browse/JDK-8234372
and related issues.

tstuefe
tstuefe approved these changes Oct 8, 2020
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

@coleenp: Looks all good to me now. Thanks for that work!

@dholmes-ora : I knew remember now the discussion as well. Seeing that it was only in February this is embarrassing :) I still find the fact the base values are off by one odd but having them at crooked values (page ends) would be odd too, so maybe its just a matter of brushing up comments.

@coleenp
Copy link
Contributor Author

coleenp commented Oct 8, 2020

Thank you for all the reviews.

/integrate

@openjdk openjdk bot closed this Oct 8, 2020
@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 8, 2020
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 8, 2020
@openjdk
Copy link

openjdk bot commented Oct 8, 2020

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

  • 782d45b: 8242882: opening jar file with large manifest might throw NegativeArraySizeException
  • f860372: 8253566: clazz.isAssignableFrom will return false for interface implementors
  • 66f27b5: 8254015: copy_to_survivor_space should use in-hand klass for scanning
  • 76a5852: 8253756: C2 CompilerThread0 crash in Node::add_req(Node*)
  • 8f9e479: 8254144: Non-x86 Zero builds fail with return-type warning in os_linux_zero.cpp
  • 7952c06: 8254166: Zero: return-type warning in zeroInterpreter_zero.cpp
  • 894ec76: 8254027: gc/g1/TestHumongousConcurrentStartUndo.java failed with "'Concurrent Mark Cycle' missing from stdout/stderr"
  • bc23690: 8254173: Add Zero, Minimal hotspot targets to submit workflow
  • e1187c4: 8248262: Wrong link target in ModuleDescriptor#isAutomatic's API documentation
  • 9cdfd0f: 8254096: remove jdk.test.lib.Utils::getMandatoryProperty(String) method
  • ... and 36 more: https://git.openjdk.java.net/jdk/compare/5d84e95ed591692818c189e640d8884b4ebd8431...master

Your commit was automatically rebased without conflicts.

Pushed as commit 6bc4931.

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

@coleenp coleenp deleted the stack-code branch October 8, 2020 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
5 participants