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

8267341: macos attempt_reserve_memory_at(arg1, arg2, true) failure #6960

Closed
wants to merge 7 commits into from
Closed

8267341: macos attempt_reserve_memory_at(arg1, arg2, true) failure #6960

wants to merge 7 commits into from

Conversation

gerard-ziemski
Copy link

@gerard-ziemski gerard-ziemski commented Jan 4, 2022

Re-enable release_multi_mappings gtest on macOS

This test would occasionally fail on macOS, but thanks to Dan's catch, it turned out that it actually only fails on those test machines that run macOS 10.13.6 or earlier.

The proposed fix simply makes a single call to os::reserve_memory() with the executable flag True, and if that failed forces executable to False later in the actual test code (alternatively we could have just also skipped that test portion completely, but this way we actually do something rather than nothing at all).

Tested manually on macOS 10.13.6 and via Mach5


Progress

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

Issue

  • JDK-8267341: macos attempt_reserve_memory_at(arg1, arg2, true) failure

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6960/head:pull/6960
$ git checkout pull/6960

Update a local copy of the PR:
$ git checkout pull/6960
$ git pull https://git.openjdk.java.net/jdk pull/6960/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6960

View PR using the GUI difftool:
$ git pr show -t 6960

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6960.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 4, 2022

👋 Welcome back gziemski! 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
Copy link

openjdk bot commented Jan 4, 2022

@gerard-ziemski The following label will be automatically applied to this pull request:

  • hotspot-runtime

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-runtime hotspot-runtime-dev@openjdk.org label Jan 4, 2022
@gerard-ziemski gerard-ziemski marked this pull request as ready for review January 4, 2022 19:41
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 4, 2022
@mlbridge
Copy link

mlbridge bot commented Jan 4, 2022

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. You mentioned "Mach5" testing; I'm presuming Tier1 was
done and passed.

test/hotspot/gtest/runtime/test_os.cpp Outdated Show resolved Hide resolved
test/hotspot/gtest/runtime/test_os.cpp Outdated Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Jan 4, 2022

@gerard-ziemski 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:

8267341: macos attempt_reserve_memory_at(arg1, arg2, true) failure

Reviewed-by: dcubed, dholmes

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

  • 5523dde: 8279641: Create manual JTReg tests for Swing accessibility
  • dac15ef: 8280182: HotSpot Style Guide has stale link to chromium style guide
  • 84fa0d8: 8190264: JScrollBar ignores its border when using macOS Mac OS X Aqua look and feel
  • 610a129: 8268831: Improve javadoc tool handling of streams.
  • e20c6bf: 8280189: JFR: TestPrintXML should print mismatching XML
  • b20b11c: 8258240: make vscode-project on Windows generates jdk.code-workspace file with unescaped '' in paths
  • 9611431: 8279936: Change shared code to use os:: system API's
  • cc2f474: 8280024: Parallel: Remove unnecessary region resizing methods in PSCardTable
  • 8931c12: 8280157: wrong texts Falied in a couple of tests
  • 68b40ec: 8273139: C2: assert(f <= 1 && f >= 0) failed: Incorrect frequency
  • ... and 178 more: https://git.openjdk.java.net/jdk/compare/99a8351bc913a94f8aebef54fe7b147545edd258...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 Jan 4, 2022
@gerard-ziemski
Copy link
Author

Thumbs up. You mentioned "Mach5" testing; I'm presuming Tier1 was
done and passed.

Thank you Dan for the review!

I did tiers: 1,2,3,4,5

@gerard-ziemski
Copy link
Author

Do I need a 2nd review? This is a test, but on the other hand we are getting pretty close to release?

If I don't get a 2nd review in the next couple of days, I will assume that I'm OK to check it in...

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.

Unless a change comes under the "trivial" rule (this does not) then yes two reviews are needed. :)

A couple of nits below but otherwise this seems a reasonable thing for the test to do.

Thanks,
David

test/hotspot/gtest/runtime/test_os.cpp Outdated Show resolved Hide resolved
test/hotspot/gtest/runtime/test_os.cpp Outdated Show resolved Hide resolved
@gerard-ziemski
Copy link
Author

Unless a change comes under the "trivial" rule (this does not) then yes two reviews are needed. :)

A couple of nits below but otherwise this seems a reasonable thing for the test to do.

Thank you David!

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.

Thanks for the refactoring. This version is cleaner.

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.

Minor nit below. Otherwise looks fine.

Thanks,
David

test/hotspot/gtest/runtime/test_os.cpp Outdated Show resolved Hide resolved
@mlbridge
Copy link

mlbridge bot commented Jan 8, 2022

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

On 8/01/2022 1:44 am, Gerard Ziemski wrote:

On Thu, 6 Jan 2022 22:59:53 GMT, David Holmes <dholmes at openjdk.org> wrote:

Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:

rename can_alloc_executable_memory to can_reserve_executable_memory

test/hotspot/gtest/runtime/test_os.cpp line 379:

377: #ifdef __APPLE__
378: // Workaround: try reserving memory with executable flag set to True
379: // to figure out if such operation is supported on this macOS version

This comment really belongs on can_reserve_executable_memory now.

I'd prefer to have a comment at the place of the workaround. How about changing it slightly to avoid implementation details like so:

// Workaround: try reserving executable memory to figure out
// if such operation is supported on this macOS version

I still think can_alloc_executable_memory requires a comment to explain
what it is doing.

David

@gerard-ziemski
Copy link
Author

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

On 8/01/2022 1:44 am, Gerard Ziemski wrote:

On Thu, 6 Jan 2022 22:59:53 GMT, David Holmes wrote:

Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
rename can_alloc_executable_memory to can_reserve_executable_memory

test/hotspot/gtest/runtime/test_os.cpp line 379:

377: #ifdef APPLE
378: // Workaround: try reserving memory with executable flag set to True
379: // to figure out if such operation is supported on this macOS version

This comment really belongs on can_reserve_executable_memory now.

I'd prefer to have a comment at the place of the workaround. How about changing it slightly to avoid implementation details like so:
// Workaround: try reserving executable memory to figure out
// if such operation is supported on this macOS version

I still think can_alloc_executable_memory requires a comment to explain what it is doing.

David

OK, I added a nice comment. What do you think?

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.

Looks good. Two minor nits.

Thanks,
David

test/hotspot/gtest/runtime/test_os.cpp Outdated Show resolved Hide resolved
test/hotspot/gtest/runtime/test_os.cpp Outdated Show resolved Hide resolved
@gerard-ziemski
Copy link
Author

Thank you David, Dan for the reviews! If you could check this one last time, then maybe I can still push it into jdk18

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.

Thumb up. Feel free to ignore my nit about adding a period.


#ifdef __APPLE__
// Workaround: try reserving executable memory to figure out
// if such operation is supported on this macOS version
Copy link
Member

Choose a reason for hiding this comment

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

Please add a period at the end of this sentence.

@gerard-ziemski
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented Jan 19, 2022

Going to push as commit d1efb0c.
Since your change was applied there have been 189 commits pushed to the master branch:

  • 6179e13: 8266410: jdk/jfr/javaagent/TestLoadedAgent.java failed with "Mismatch in TestEvent count"
  • 5523dde: 8279641: Create manual JTReg tests for Swing accessibility
  • dac15ef: 8280182: HotSpot Style Guide has stale link to chromium style guide
  • 84fa0d8: 8190264: JScrollBar ignores its border when using macOS Mac OS X Aqua look and feel
  • 610a129: 8268831: Improve javadoc tool handling of streams.
  • e20c6bf: 8280189: JFR: TestPrintXML should print mismatching XML
  • b20b11c: 8258240: make vscode-project on Windows generates jdk.code-workspace file with unescaped '' in paths
  • 9611431: 8279936: Change shared code to use os:: system API's
  • cc2f474: 8280024: Parallel: Remove unnecessary region resizing methods in PSCardTable
  • 8931c12: 8280157: wrong texts Falied in a couple of tests
  • ... and 179 more: https://git.openjdk.java.net/jdk/compare/99a8351bc913a94f8aebef54fe7b147545edd258...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Jan 19, 2022

@gerard-ziemski Pushed as commit d1efb0c.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
3 participants