Skip to content

8347830: [premain] UseCompatibleCompressedOops is broken after merging with mainline #67

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

Open
wants to merge 2 commits into
base: premain
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 20 additions & 16 deletions src/hotspot/share/cds/cdsConfig.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,26 @@ void CDSConfig::ergo_initialize() {
if (!is_dumping_heap()) {
_is_dumping_full_module_graph = false;
}

#ifdef _LP64
//
// By default, when using AOTClassLinking, use the CompressedOops::HeapBasedNarrowOop
// mode so that AOT code can be always work regardless of runtime heap range.
//
// If you are *absolutely sure* that the CompressedOops::mode() will be the same
// between training and production runs (e.g., if you specify -Xmx128m for
// both training and production runs, and you know the OS will always reserve
// the heap under 4GB), you can explicitly disable this with:
// java -XX:+UnlockDiagnosticVMOptions -XX:-UseCompatibleCompressedOops ...
// However, this is risky and there's a chance that the production run will be slower
// than expected because it is unable to load the AOT code cache.
//
if (UseCompressedOops && AOTCodeCache::is_caching_enabled()) {
FLAG_SET_ERGO_IF_DEFAULT(UseCompatibleCompressedOops, true);
} else if (!FLAG_IS_DEFAULT(UseCompatibleCompressedOops)) {
FLAG_SET_ERGO(UseCompatibleCompressedOops, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to set it to false explicitly here? If it is not default shouldn't we use whatever value the user specified?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I consider the flag make sense only for AOT code and not for general usage. See description: globals.hpp#L132

We don't want generate more complex encoding if we don't generate/use AOT code. Or compressed oops are off.
We had also recently discussed how we should disable some AOT features if AOT caching is off - the conclusion was that we should silently ignore them (may be produce UL output on demand). That is why I used ERGO setting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay

}
#endif // _LP64
}

const char* CDSConfig::default_archive_path() {
Expand Down Expand Up @@ -765,22 +785,6 @@ void CDSConfig::setup_compiler_args() {
// Ergo set-up of various flags used by the experimental workflow that uses -XX:CacheDataStore. This workflow
// is deprecated and will be removed from Leyden.
bool CDSConfig::setup_experimental_leyden_workflow(bool xshare_auto_cmd_line) {
// Leyden temp work-around:
//
// By default, when using CacheDataStore, use the HeapBasedNarrowOop mode so that
// AOT code can be always work regardless of runtime heap range.
//
// If you are *absolutely sure* that the CompressedOops::mode() will be the same
// between training and production runs (e.g., if you specify -Xmx128m
// for both training and production runs, and you know the OS will always reserve
// the heap under 4GB), you can explicitly disable this with:
// java -XX:-UseCompatibleCompressedOops -XX:CacheDataStore=...
// However, this is risky and there's a chance that the production run will be slower
// because it is unable to load the AOT code cache.
#ifdef _LP64
// FLAG_SET_ERGO_IF_DEFAULT(UseCompatibleCompressedOops, true); // FIXME @iklam - merge with mainline - UseCompatibleCompressedOops
#endif

if (FLAG_IS_DEFAULT(AOTClassLinking)) {
FLAG_SET_ERGO(AOTClassLinking, true);
}
Expand Down
10 changes: 10 additions & 0 deletions src/hotspot/share/code/aotCodeCache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4088,6 +4088,16 @@ int AOTCodeAddressTable::id_for_address(address addr, RelocIterator reloc, CodeB
if (addr == (address)-1) { // Static call stub has jump to itself
return id;
}
// Check card_table_base address first since it can point to any address
BarrierSet* bs = BarrierSet::barrier_set();
if (bs->is_a(BarrierSet::CardTableBarrierSet)) {
if (addr == ci_card_table_address_as<address>()) {
id = search_address(addr, _extrs_addr, _extrs_length);
assert(id > 0 && _extrs_addr[id - _extrs_base] == addr, "sanity");
return id;
}
}

// Seach for C string
id = id_for_C_string(addr);
if (id >= 0) {
Expand Down
25 changes: 17 additions & 8 deletions src/hotspot/share/memory/memoryReserver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,8 @@ static ReservedSpace establish_noaccess_prefix(const ReservedSpace& reserved, si
assert(reserved.alignment() >= os::vm_page_size(), "must be at least page size big");
assert(reserved.is_reserved(), "should only be called on a reserved memory area");

if (reserved.end() > (char *)OopEncodingHeapMax) {
if (reserved.end() > (char *)OopEncodingHeapMax || UseCompatibleCompressedOops) {
assert((reserved.base() != nullptr), "sanity");
if (true
WIN64_ONLY(&& !UseLargePages)
AIX_ONLY(&& (os::Aix::supports_64K_mmap_pages() || os::vm_page_size() == 4*K))) {
Expand Down Expand Up @@ -549,13 +550,20 @@ ReservedHeapSpace HeapReserver::Instance::reserve_compressed_oops_heap(const siz
const size_t attach_point_alignment = lcm(alignment, os_attach_point_alignment);

char* aligned_heap_base_min_address = align_up((char*)HeapBaseMinAddress, alignment);
size_t noaccess_prefix = ((aligned_heap_base_min_address + size) > (char*)OopEncodingHeapMax) ?
noaccess_prefix_size : 0;
char* heap_end_address = aligned_heap_base_min_address + size;

bool unscaled = false;
bool zerobased = false;
if (!UseCompatibleCompressedOops) { // heap base is not enforced
unscaled = (heap_end_address <= (char*)UnscaledOopHeapMax);
zerobased = (heap_end_address <= (char*)OopEncodingHeapMax);
}
size_t noaccess_prefix = !zerobased ? noaccess_prefix_size : 0;

ReservedSpace reserved{};

// Attempt to alloc at user-given address.
if (!FLAG_IS_DEFAULT(HeapBaseMinAddress)) {
if (!FLAG_IS_DEFAULT(HeapBaseMinAddress) || UseCompatibleCompressedOops) {
reserved = try_reserve_memory(size + noaccess_prefix, alignment, page_size, aligned_heap_base_min_address);
if (reserved.base() != aligned_heap_base_min_address) { // Enforce this exact address.
release(reserved);
Expand All @@ -577,7 +585,7 @@ ReservedHeapSpace HeapReserver::Instance::reserve_compressed_oops_heap(const siz

// Attempt to allocate so that we can run without base and scale (32-Bit unscaled compressed oops).
// Give it several tries from top of range to bottom.
if (aligned_heap_base_min_address + size <= (char *)UnscaledOopHeapMax) {
if (unscaled) {

// Calc address range within we try to attach (range of possible start addresses).
char* const highest_start = align_down((char *)UnscaledOopHeapMax - size, attach_point_alignment);
Expand All @@ -590,9 +598,9 @@ ReservedHeapSpace HeapReserver::Instance::reserve_compressed_oops_heap(const siz
char *zerobased_max = (char *)OopEncodingHeapMax;

// Give it several tries from top of range to bottom.
if (aligned_heap_base_min_address + size <= zerobased_max && // Zerobased theoretical possible.
((!reserved.is_reserved()) || // No previous try succeeded.
(reserved.end() > zerobased_max))) { // Unscaled delivered an arbitrary address.
if (zerobased && // Zerobased theoretical possible.
((!reserved.is_reserved()) || // No previous try succeeded.
(reserved.end() > zerobased_max))) { // Unscaled delivered an arbitrary address.

// Release previous reservation
release(reserved);
Expand Down Expand Up @@ -658,6 +666,7 @@ ReservedHeapSpace HeapReserver::Instance::reserve_compressed_oops_heap(const siz
}

// We reserved heap memory without a noaccess prefix.
assert(!UseCompatibleCompressedOops, "noaccess prefix is missing");
return ReservedHeapSpace(reserved, 0 /* noaccess_prefix */);
}

Expand Down