[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8230664: Fix TestInstanceKlassSize for PowerPC [v2]
From:       Chris Plummer <cjplummer () openjdk ! java ! net>
Date:       2020-09-30 23:39:51
Message-ID: DWoieErFHSC8fLBZ5p0uO8DVE0Ddw8Mz20B_1y1G95s=.562e1a9d-e301-4ab3-ac44-471c64892965 () github ! com
[Download RAW message or body]

On Wed, 30 Sep 2020 21:28:19 GMT, Ziviani <github.com+670087+jrziviani@openjdk.org> \
wrote:

> > TestInstanceKlassSize was failing because, for PowerPC, the following code \
> > (instanceKlass.cpp) always compiles to `return false;` bool \
> > InstanceKlass::has_stored_fingerprint() const { #if INCLUDE_AOT
> > return should_store_fingerprint() || is_shared();
> > #else
> > return false;
> > #endif
> > }
> > However, in `hasStoredFingerprint()@InstanceKlass.java` the condition \
> > `shouldStoreFingerprint() || isShared();` is always evaluated and may return true \
> > (_AFAIK isShared() returns true_). Such condition adds 8 bytes in the \
> > `getSize()@InstanceKlass.java` causing the failure in TestInstanceKlassSize: \
> > public long getSize() { // in number of bytes
> > ...
> > if (hasStoredFingerprint()) {
> > size += 8; // uint64_t
> > }
> > return alignSize(size);
> > }
> > Considering these tests are failing for PowerPC only (_based on \
> > ProblemList.txt_), my solution checks if `hasStoredFingerprint()` is running on a \
> > PowerPC platform. I decided to go this way because there is no existing flag \
> > informing whether AOT is included or not and creating a new one just to handle \
> > the PowerPC case seems too much.  This patch is an attempt to fix \
> > https://bugs.openjdk.java.net/browse/JDK-8230664
> 
> Ziviani has updated the pull request with a new target base due to a merge or a \
> rebase. The pull request now contains one commit:
> 8230664: Fix TestInstanceKlassSize
> 
> The code hasStoredFingerprint() at InstanceKlass.java is not considering
> AOT disabled at compilation time, like has_stored_fingerprint() at
> instanceKlass.cpp does. Such difference can cause TestInstanceKlassSize
> failures because all objects will have an extra 8-bytes.

Changes look good.

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

Marked as reviewed by cjplummer (Reviewer).

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


[prev in list] [next in list] [prev in thread] [next in thread]