RFR: JDK-8241602 jlink does not produce reproducible jimage files

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8241602 jlink does not produce reproducible jimage files

Jim Laskey (Oracle)
This fix addresses the inconsistent ordering by jimage content by jlink from run to run. Bottom line, the implementer was using HashSet without defining hashcode/equals for the Set entry classes.

webrev: http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00 <http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00>
jbs: https://bugs.openjdk.java.net/browse/JDK-8241602 <https://bugs.openjdk.java.net/browse/JDK-8241602>

Cheers,

-- Jim

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8241602 jlink does not produce reproducible jimage files

Magnus Ihse Bursie
On 2020-05-05 21:56, Jim Laskey wrote:
> This fix addresses the inconsistent ordering by jimage content by jlink from run to run. Bottom line, the implementer was using HashSet without defining hashcode/equals for the Set entry classes.
>
> webrev: http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00 <http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00>
Looks good to me.

/Magnus
> jbs: https://bugs.openjdk.java.net/browse/JDK-8241602 <https://bugs.openjdk.java.net/browse/JDK-8241602>
>
> Cheers,
>
> -- Jim
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8241602 jlink does not produce reproducible jimage files

Alan Bateman
In reply to this post by Jim Laskey (Oracle)
On 05/05/2020 20:56, Jim Laskey wrote:
> This fix addresses the inconsistent ordering by jimage content by jlink from run to run. Bottom line, the implementer was using HashSet without defining hashcode/equals for the Set entry classes.
>
> webrev: http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00 <http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00>
> jbs: https://bugs.openjdk.java.net/browse/JDK-8241602 <https://bugs.openjdk.java.net/browse/JDK-8241602>
>
DirArchive and JmodArchive look okay. They could use Objects.hash/equals
but what you have is okay too. Can you re-check JarArchive as it is
created with a runtime version so equals should be checking 3 fields.

The existing test for reproducible builds is JLinkReproducibleTest.
Adding a new test is okay too but we should at least try to keep the
names consistent. A couple of suggestions for the test in the webrev:
- the @modules tag needs to include java.desktop as it is needed by the
test. Better still would be to change to java.se so that there are more
modules in the images.
- Did you mean to open jdk.tools.jlink.internal? Maybe its a leftover
from a previous iteration of the test?
- You can use Files.mismatch to compare the lib/modules files are
identical (like JLinkReproducibleTest).  It's okay to use the jimage
ImageReaderFactory to check the names tables too but I think it's more
important to check that the lib/modules files are identical before
probing further.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8241602 jlink does not produce reproducible jimage files

Jim Laskey (Oracle)
Thank you. Notes below.


updated webrev: http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00 <http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00>


> On May 6, 2020, at 4:14 AM, Alan Bateman <[hidden email]> wrote:
>
> On 05/05/2020 20:56, Jim Laskey wrote:
>> This fix addresses the inconsistent ordering by jimage content by jlink from run to run. Bottom line, the implementer was using HashSet without defining hashcode/equals for the Set entry classes.
>>
>> webrev: http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00 <http://cr.openjdk.java.net/~jlaskey/8241602/webrev-00>
>> jbs: https://bugs.openjdk.java.net/browse/JDK-8241602 <https://bugs.openjdk.java.net/browse/JDK-8241602>
>>
> DirArchive and JmodArchive look okay. They could use Objects.hash/equals but what you have is okay too. Can you re-check JarArchive as it is created with a runtime version so equals should be checking 3 fields.

Opted to use Objects. Added version for JarArchive (not convinced needed but should be benign.)

>
> The existing test for reproducible builds is JLinkReproducibleTest. Adding a new test is okay too but we should at least try to keep the names consistent. A couple of suggestions for the test in the webrev:
> - the @modules tag needs to include java.desktop as it is needed by the test. Better still would be to change to java.se so that there are more modules in the images.

Switched to  java.se, only a second slower than java.desktop . Overall better test than JLinkReproducibleTest for increased likelihood of variance.

> - Did you mean to open jdk.tools.jlink.internal? Maybe its a leftover from a previous iteration of the test?

Nope, Yep.

> - You can use Files.mismatch to compare the lib/modules files are identical (like JLinkReproducibleTest).  It's okay to use the jimage ImageReaderFactory to check the names tables too but I think it's more important to check that the lib/modules files are identical before probing further.

I had it in my mind there was a timestamp in the file, but then I remembered there wasn't. Switched to Files.mismatch. Everything else is redundant.

Aside: The order in the file is still somewhat scattered, based on archive and archive content.  We have the a pattern based sorting plugin which we don't use and we never did any locality vs performance testing.

>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8241602 jlink does not produce reproducible jimage filesJ

Alan Bateman
On 06/05/2020 14:42, Jim Laskey wrote:
> :
>
> Aside: The order in the file is still somewhat scattered, based on
> archive and archive content.  We have the a pattern based sorting
> plugin which we don't use and we never did any locality vs performance
> testing.
>
Not in the test but the JDK does use the order-resources plugin with the
following pattern:

JLINK_ORDER_RESOURCES += \
     /java.base/java/** \
     /java.base/jdk/** \
     /java.base/sun/** \
     /java.base/com/** \
     /jdk.localedata/** \
     #

Anyway, I looked at webrev-01 and the jlink changes look good. The
updated test doesn't use the jimage API so no need to open
jdk.internal.jimage. The imports can be removed too, also RUNTIME is no
longer used in this version. Have you decided on a test name? The
rallying call is "reproducible builds" and I'd prefer to have
"Reproducible" in the test name so its consistent with the existing test
that checks for reproducible with user modules.

-Alan.


Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8241602 jlink does not produce reproducible jimage filesJ

Jim Laskey (Oracle)
http://cr.openjdk.java.net/~jlaskey/8241602/webrev-02 <http://cr.openjdk.java.net/~jlaskey/8241602/webrev-02>


> On May 6, 2020, at 11:05 AM, Alan Bateman <[hidden email]> wrote:
>
> On 06/05/2020 14:42, Jim Laskey wrote:
>> :
>>
>> Aside: The order in the file is still somewhat scattered, based on archive and archive content.  We have the a pattern based sorting plugin which we don't use and we never did any locality vs performance testing.
>>
> Not in the test but the JDK does use the order-resources plugin with the following pattern:
>
> JLINK_ORDER_RESOURCES += \
>     /java.base/java/** \
>     /java.base/jdk/** \
>     /java.base/sun/** \
>     /java.base/com/** \
>     /jdk.localedata/** \
>     #
>
> Anyway, I looked at webrev-01 and the jlink changes look good. The updated test doesn't use the jimage API so no need to open jdk.internal.jimage. The imports can be removed too, also RUNTIME is no longer used in this version. Have you decided on a test name? The rallying call is "reproducible builds" and I'd prefer to have "Reproducible" in the test name so its consistent with the existing test that checks for reproducible with user modules.
>
> -Alan.
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8241602 jlink does not produce reproducible jimage filesJ

Alan Bateman
On 06/05/2020 15:45, Jim Laskey wrote:
> http://cr.openjdk.java.net/~jlaskey/8241602/webrev-02
This version looks okay to me.

-Alan.