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

8237363: Remove automatic is in heap verification in OopIterateClosure #797

Closed

Conversation

stefank
Copy link
Member

@stefank stefank commented Oct 22, 2020

There's verification code in the "oop iterate" framework that asserts that a pointer is "is in the heap". This works for most GCs, but ZGC can eagerly decommit the old relocation set pages, which means that pointers to the old / from copy of the object could point to memory that is currently not a part of the current heap.

To combat this in the past I've added a way for oop iterate closures to turn off this verification. However, every single time we add a new closure we have to consider if we can allow this verification check or if we have to remove it. Personally, I think this is a false abstraction and also widens the oop iterate closure interface. I previously proposed a patch that moved the verification code down into the oop iterate closures. It wasn't a huge patch, but I got push-back that it was convenient for other GCs to get this automatic verification, and the review stalled.

In this new patch I propose a different way to retain the verification. The realization is that most oop iterate closures have to deal with both compressed and non-compressed oops, so the code typically looks like this:

template <class T>
inline void G1ScanCardClosure::do_oop_work(T* p) {
  T o = RawAccess<>::oop_load(p);
  if (CompressedOops::is_null(o)) {
    return;
  }
  oop obj = CompressedOops::decode_not_null(o);

Therefore the suggest new place to put the is_in verification is in the CompressedOops::decode*. This injects the assert into almost all non-ZGC closures, and also to places that don't use the oop iterate closure framework. I think this is a neat workaround, and hope this patch is accepted this time.

I've tested this patch a few weeks ago, but will rerun the relevant tiers.


Progress

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

Testing

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

Failed test task

Issue

  • JDK-8237363: Remove automatic is in heap verification in OopIterateClosure

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 22, 2020

👋 Welcome back stefank! 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.

@stefank
Copy link
Member Author

stefank commented Oct 22, 2020

This is mostly of concerns for the hotspot-gc, but touches compressed oops so I'll move this to hotspot instead.

/label add hotspot

@openjdk openjdk bot added rfr Pull request is ready for review hotspot hotspot-dev@openjdk.org labels Oct 22, 2020
@openjdk
Copy link

openjdk bot commented Oct 22, 2020

@stefank
The hotspot label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Oct 22, 2020

Webrevs

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

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

This has two minor drawbacks for GC implementations that verify oops with their own asserts (like Shenandoah): they would call into CollectedHeap::is_in twice (once from shared code assert, and once from their own), and then also fail with non-rich assert (in the shared code) when something goes wrong. Of course, that can be mitigated by calling into _raw versions.

@@ -1737,7 +1737,7 @@ address FileMapInfo::decode_start_address(FileMapRegion* spc, bool with_current_
size_t offset = spc->mapping_offset();
narrowOop n = CompressedOops::narrow_oop_cast(offset);
if (with_current_oop_encoding_mode) {
return cast_from_oop<address>(CompressedOops::decode_not_null(n));
return cast_from_oop<address>(CompressedOops::decode_raw_not_null(n));
Copy link
Member

Choose a reason for hiding this comment

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

Why does this line skip verification now?

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is used to setup the CDS archive. At that point in time, the heap isn't mapped yet, and there's no heap region at the suggested offset, so the new assert fails.

HeapRegion::is_in_reserved (this=0x0, p=0x7bfe00000)
HeapRegion::is_in (this=0x0, p=0x7bfe00000)
G1CollectedHeap::is_in (this=0x7ffff003a100, p=0x7bfe00000)
CompressedOops::decode_not_null (v=(unknown: -134479872))
FileMapInfo::decode_start_address (this=0x7ffff05d3e60, spc=0x7ffff05d4000, with_current_oop_encoding_mode=true)
FileMapInfo::start_address_as_decoded_with_current_oop_encoding_mode
FileMapInfo::get_heap_regions_range_with_current_oop_encoding_mode
FileMapInfo::map_heap_regions_impl
FileMapInfo::map_heap_regions
MetaspaceShared::map_archives
MetaspaceShared::initialize_runtime_shared_and_meta_spaces ()
Metaspace::global_initialize
universe_init ()
init_globals ()
Threads::create_vm

Note that the heap region is null (this=0x0). This also mean that the returned oop is not actually a valid oop at all, and this can sort of be seen by the immediate cast to address.

Copy link
Member

Choose a reason for hiding this comment

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

Ew. That seems to imply that coops decoding is now tied to heap initialization for these asserts to work. I am pleasantly surprised it only fails in one place! I guess it is fine.

@stefank
Copy link
Member Author

stefank commented Oct 22, 2020

This has two minor drawbacks for GC implementations that verify oops with their own asserts (like Shenandoah): they would call into CollectedHeap::is_in twice (once from shared code assert, and once from their own), and then also fail with non-rich assert (in the shared code) when something goes wrong. Of course, that can be mitigated by calling into _raw versions.

Yes, those are the trade-offs. Do you consider this a blocker?

I personally wouldn't mind completely removing this verification from the shared code, and let all the GCs do their own oop verification. But others have had the opposite opinion.

@stefank stefank changed the title 8237363 remove oop iterate verification 8237363: Remove automatic is in heap verification in OopIterateClosure Oct 22, 2020
@shipilev
Copy link
Member

This has two minor drawbacks for GC implementations that verify oops with their own asserts (like Shenandoah): they would call into CollectedHeap::is_in twice (once from shared code assert, and once from their own), and then also fail with non-rich assert (in the shared code) when something goes wrong. Of course, that can be mitigated by calling into _raw versions.

Yes, those are the trade-offs. Do you consider this a blocker?

Not really a blocker, just mentioning the follow-up work that might need to be done.

On one hand, we almost never omit asserts on performance grounds, but on the other hand, this adds assert on a rather frequent path. I don't mind having an extra safety there, and then let callers (e.g. GCs) to use _raw versions to make fastdebug testing a tad faster.

@stefank
Copy link
Member Author

stefank commented Oct 22, 2020

This has two minor drawbacks for GC implementations that verify oops with their own asserts (like Shenandoah): they would call into CollectedHeap::is_in twice (once from shared code assert, and once from their own), and then also fail with non-rich assert (in the shared code) when something goes wrong. Of course, that can be mitigated by calling into _raw versions.

Yes, those are the trade-offs. Do you consider this a blocker?

Not really a blocker, just mentioning the follow-up work that might need to be done.

On one hand, we almost never omit asserts on performance grounds, but on the other hand, this adds assert on a rather frequent path. I don't mind having an extra safety there, and then let callers (e.g. GCs) to use _raw versions to make fastdebug testing a tad faster.

OK. Thanks.

Copy link
Contributor

@fisk fisk 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. Great work!

@openjdk
Copy link

openjdk bot commented Oct 22, 2020

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

8237363: Remove automatic is in heap verification in OopIterateClosure

Reviewed-by: eosterlund, pliden

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

  • e8b75b1: 8255393: sun/security/util/DerValue/Indefinite.java fails with ---illegal-access=deny
  • 7cafe35: 8255352: Archive important test outputs in submit workflow
  • 888086f: 8255373: Submit workflow artifact name is always "test-results_.zip"
  • 6918818: 8255265: IdealLoopTree::iteration_split_impl does not use should_align
  • c28b011: 8255343: java/util/stream/SpliteratorTest.java fails on 32-bit platforms with "Misaligned access at address: 12"
  • b71b5b4: 8199062: Test javax/swing/text/Utilities/8134721/bug8134721.java is unstable
  • ee34fa5: 8199060: Test javax/swing/text/html/parser/Parser/6990651/bug6990651.java is unstable
  • 93dadbe: 7190589: [macosx] In the test bug4278839 never press ctrl+arrow
  • 57d903b: 8255242: Bidi.requiresBidi has misleading exception message
  • 60d0142: 8255379: ProblemList compiler/loopstripmining/BackedgeNodeWithOutOfLoopControl.java
  • ... and 36 more: https://git.openjdk.java.net/jdk/compare/b25d8940156b106b1ffef9a239e0f002d1397335...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 22, 2020
@stefank
Copy link
Member Author

stefank commented Oct 26, 2020

/integrate

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

openjdk bot commented Oct 26, 2020

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

  • fa64477: 8255301: Common and strengthen the code in ciMemberName and ciMethodHandle
  • 9b5a2a6: 8255349: Vector API issues on Big Endian
  • e8b75b1: 8255393: sun/security/util/DerValue/Indefinite.java fails with ---illegal-access=deny
  • 7cafe35: 8255352: Archive important test outputs in submit workflow
  • 888086f: 8255373: Submit workflow artifact name is always "test-results_.zip"
  • 6918818: 8255265: IdealLoopTree::iteration_split_impl does not use should_align
  • c28b011: 8255343: java/util/stream/SpliteratorTest.java fails on 32-bit platforms with "Misaligned access at address: 12"
  • b71b5b4: 8199062: Test javax/swing/text/Utilities/8134721/bug8134721.java is unstable
  • ee34fa5: 8199060: Test javax/swing/text/html/parser/Parser/6990651/bug6990651.java is unstable
  • 93dadbe: 7190589: [macosx] In the test bug4278839 never press ctrl+arrow
  • ... and 38 more: https://git.openjdk.java.net/jdk/compare/b25d8940156b106b1ffef9a239e0f002d1397335...master

Your commit was automatically rebased without conflicts.

Pushed as commit 6666dcb.

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

@stefank stefank deleted the 8237363_remove_oop_iterate_verification branch October 27, 2020 08:46
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
4 participants