RFR: 8237363: Remove automatic is in heap verification in OopIterateClosure

[Aleksey Shipilev]

On Thu, 22 Oct 2020 08:54:27 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> > 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.

>> src/hotspot/share/memory/filemap.cpp line 1740:
>> 
>>> 1738:   narrowOop n = CompressedOops::narrow_oop_cast(offset);
>>> 1739:   if (with_current_oop_encoding_mode) {
>>> 1740:     return cast_from_oop<address>(CompressedOops::decode_raw_not_null(n));
>> 
>> Why does this line skip verification now?
>
> 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.

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/797