Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v2]
> Both jar and jmod utilise java.io file operations whose methods define no > ordering of the return file lists, and in fact rely on OS query file > ordering, which can differ by underlying OS architecture. > This PR adds sort processing to the creation of such jar's and jmod's to > enable a deterministic content ordering. > > Signed-off-by: Andrew Leonard Andrew Leonard has updated the pull request incrementally with two additional commits since the last revision: - 8276764: Enable deterministic file content ordering for Jar and Jmod Signed-off-by: Andrew Leonard - 8276764: Enable deterministic file content ordering for Jar and Jmod Signed-off-by: Andrew Leonard - Changes: - all: https://git.openjdk.java.net/jdk/pull/6395/files - new: https://git.openjdk.java.net/jdk/pull/6395/files/3e87db19..772b89a4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=00-01 Stats: 227 lines in 3 files changed: 201 ins; 1 del; 25 mod Patch: https://git.openjdk.java.net/jdk/pull/6395.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6395/head:pull/6395 PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v2]
On Thu, 18 Nov 2021 23:49:46 GMT, Mandy Chung wrote: >> Andrew Leonard has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - 8276764: Enable deterministic file content ordering for Jar and Jmod >> >>Signed-off-by: Andrew Leonard >> - 8276764: Enable deterministic file content ordering for Jar and Jmod >> >>Signed-off-by: Andrew Leonard > > src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 130: > >> 128: >> 129: // There's also a files array per version >> 130: // Use a LinkedHashMap to keep original insertion ordering > > Suggestion: > > // base version is the first entry and then follow with the version given > // from the --release option in the command-line order. > // The value of each entry is the files given in the command-line order. done - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v2]
On Thu, 18 Nov 2021 09:32:04 GMT, Andrew Leonard wrote:
>> test/jdk/tools/jar/ContentOrder.java line 79:
>>
>>> 77: @Test
>>> 78: public void test1() throws IOException {
>>> 79: mkdir("testjar/Ctest1 testjar/Btest2/subdir1 testjar/Atest3");
>>
>> I suggest to make `mkdir` and `touch` to take a vararg of String or Path and
>> the method body concatenates the input strings to build the command line.
>
> I copied this from another test, but your suggestion makes sense
fixed as suggested
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v2]
On Wed, 17 Nov 2021 21:29:00 GMT, Lance Andersen wrote:
>> I agree, I always use {}, I was being consistent with the rest of the
>> expand() method, but I will add {} as I prefer that too, better to set a
>> good example!
>
> Feel free to clean those up as well :-)
all fixed
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
> Both jar and jmod utilise java.io file operations whose methods define no > ordering of the return file lists, and in fact rely on OS query file > ordering, which can differ by underlying OS architecture. > This PR adds sort processing to the creation of such jar's and jmod's to > enable a deterministic content ordering. > > Signed-off-by: Andrew Leonard Andrew Leonard has updated the pull request incrementally with one additional commit since the last revision: 8276764: Enable deterministic file content ordering for Jar and Jmod Signed-off-by: Andrew Leonard - Changes: - all: https://git.openjdk.java.net/jdk/pull/6395/files - new: https://git.openjdk.java.net/jdk/pull/6395/files/772b89a4..c8d6e90d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=01-02 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6395.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6395/head:pull/6395 PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v3]
On Wed, 17 Nov 2021 19:48:25 GMT, kabutz wrote:
>> BigInteger currently uses three different algorithms for multiply. The
>> simple quadratic algorithm, then the slightly better Karatsuba if we exceed
>> a bit count and then Toom Cook 3 once we go into the several thousands of
>> bits. Since Toom Cook 3 is a recursive algorithm, it is trivial to
>> parallelize it. I have demonstrated this several times in conference talks.
>> In order to be consistent with other classes such as Arrays and Collection,
>> I have added a parallelMultiply() method. Internally we have added a
>> parameter to the private multiply method to indicate whether the calculation
>> should be done in parallel.
>>
>> The performance improvements are as should be expected. Fibonacci of 100
>> million (using a single-threaded Dijkstra's sum of squares version)
>> completes in 9.2 seconds with the parallelMultiply() vs 25.3 seconds with
>> the sequential multiply() method. This is on my 1-8-2 laptop. The final
>> multiplications are with very large numbers, which then benefit from the
>> parallelization of Toom-Cook 3. Fibonacci 100 million is a 347084 bit number.
>>
>> We have also parallelized the private square() method. Internally, the
>> square() method defaults to be sequential.
>>
>> Some benchmark results, run on my 1-6-2 server:
>>
>>
>> Benchmark (n) Mode Cnt Score
>> Error Units
>> BigIntegerParallelMultiply.multiply100ss4 51.707
>> ± 11.194 ms/op
>> BigIntegerParallelMultiply.multiply 1000ss4988.302
>> ± 235.977 ms/op
>> BigIntegerParallelMultiply.multiply 1ss4 24662.063
>> ± 1123.329 ms/op
>> BigIntegerParallelMultiply.parallelMultiply100ss4 49.337
>> ± 26.611 ms/op
>> BigIntegerParallelMultiply.parallelMultiply 1000ss4527.560
>> ± 268.903 ms/op
>> BigIntegerParallelMultiply.parallelMultiply 1ss4 9076.551
>> ± 1899.444 ms/op
>>
>>
>> We can see that for larger calculations (fib 100m), the execution is 2.7x
>> faster in parallel. For medium size (fib 10m) it is 1.873x faster. And for
>> small (fib 1m) it is roughly the same. Considering that the fibonacci
>> algorithm that we used was in itself sequential, and that the last 3
>> calculations would dominate, 2.7x faster should probably be considered quite
>> good on a 1-6-2 machine.
>
> kabutz has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Removed JVM flags from benchmark
> I think the functionality in this PR is worth pursuing, but with the JDK 18
> rampdown 1 date fast approaching, as a non-urgent issue, I think we shouldn't
> try to rush it into JDK 18.
> I looked more closely and now understand a bit more about the threshold. It
> would be useful to have some implementation notes detailing approximately
> when the parallel execution kicks in. For `a*a` it's `>=
> TOOM_COOK_SQUARE_THRESHOLD(216)` and for `a*b` it's `>=
> TOOM_COOK_THRESHOLD(240)`, so we could refer to certain bit lengths.
>
> The branching factor is 4 (well 3 i guess, but its easier to think in powers
> of 2). It might be reasonable to assume the problem gets split equally in 4
> parts. I don't know in practice what the depth of recursion might, its hard
> to see this getting completely out of control, but we could always keep track
> of the depth and cut off in proportion to the # runtime processors if need be.
>
> Given the existing degree of specialization, the minimal changes to code, and
> the fact that the creation of recursive tasks is in the noise this PR looks
> quite reasonable.
I have run some more tests. For my fibonacci algorithm, here are the worst
cases for the various calculations.
n most_bits tasks time_ms
1000 694 0 1
10_000 6_942 0 1
100_000 69_42418 13
1_000_000 694_241 468143
10_000_000 6_942_41811_718 1049
100_000_000 69_424_191 292_968 13695
1_000_000_000 694_241_913 7_324_218 237282
Each data point has 10x the number of bits in the final result and the number
of tasks in the final calculation is 25x more. Perhaps I can make the threshold
the recursive depth up to which we would run in parallel. And that recursive
depth could the availableProcessors() or some multiple thereof.
>
> I think we can simplify the creation of the recursive tasks (assuming we
> don't track the depth):
>
> ```java
> private static final class RecursiveOp {
> private static RecursiveTask exec(RecursiveTask op, boolean
> parallel) {
> if (parallel) {
> op.fork();
> } else {
> op.invoke();
> }
> return op;
> }
>
> private static RecursiveTask multiply(
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v3]
On Fri, 19 Nov 2021 05:12:25 GMT, Vicente Romero wrote: > It seems like the execution is way faster for these number of slots. I suggested this experiment to split up the concatenations more aggressively to diagnose if we're having an issue here where the performance of the method handle produced by `StringConcatFactory` degrades after some number of arguments compared to a more traditional concatenation (as exemplified by the Lombok valueClass tests). And that seems to be the case. While I haven't analyzed the root cause here (I think it seems likely that the JIT bails out at some point when the MH combinator tree grows too large or more likely too deep (long?)) I'd characterize this is a performance bug in the `StringConcatFactory`. This might be straightforward to address by moving the splitting logic you do here inside `StringConcatFactory`. This should be done as a follow-up. Assuming all you had to do to improve over the current patch was set your local `MAX_INDY_CONCAT_ARG_SLOTS` constant to 20 I think it's fine to put that in - we'll want to look at simplifying the `ObjectMethods` code once `StringConcatFactory` better deals with many arguments anyhow. - PR: https://git.openjdk.java.net/jdk/pull/6403
Seeking inputs on 8224794: ZipFile/JarFile should open zip/JAR file with FILE_SHARE_DELETE sharing mode on Windows
I have been experimenting with https://bugs.openjdk.java.net/browse/JDK-8224794 and have reached a stage with my changes where I would like some inputs before I move ahead or raise a PR for review. As noted in that JBS issue, the idea is to open the underlying file using FILE_SHARE_DELETE when using java.uitl.zip.ZipFile and java.util.jar.JarFile. From what I can infer from that JBS issue, the motivation behind this seems to be to allow deleting that underlying file when that file is currently opened and in use by the ZipFile/JarFile instance. The linked github issues seem to suggest the same. IMO, it's a reasonable expectation, given that such a deletion works on *nix platform. It's on Windows where such a deletion fails with "The process cannot access the file because it is being used by another process" (the other process in many cases will be the current JVM instance itself). So trying to get this to work on Windows by setting the FILE_SHARE_DELETE share access mode seems reasonable. Coming to the implementation part, I've an initial draft version here https://github.com/openjdk/jdk/pull/6477. It sets the FILE_SHARE_DELETE share access mode on the file when constructed by ZipFile/JarFile. It "works", in the sense that if you issue a delete against this underlying file (path) after its been opened by ZipFile/JarFile instance, you no longer see the error mentioned above and the delete completes without any exception. However, my experiments with this change have raised a bunch of questions related to this: The crucial part of the "DeleteFile" API in Windows is that, as stated in its documentation[1]: "The DeleteFile function marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED." So imagine the following pseudo-code: String path = "C:/foo.jar" 1. JarFile jar = new JarFile(path); // create a jar instance 2. var deleted = new File(path).delete(); // delete the underlying file 3. var exists = new File(path).exists(); // check file existence 4. FileOutputStream fos = new FileOutputStream(new File(path)); // open a FileOutputStream for /tmp/foo.jar 5. java.nio.file.Files.deleteIfExists(Path.of(path)); When this is run (on Windows) *without* the change that introduces FILE_SHARE_DELETE, I notice that line#2 returns "false" for File.delete() (since the file is in use by the JarFile instance) and then line#3 returns "true" since it still exists and line#4 succeeds and creates a new FileOutputStream and line#5 fails with an exception stating "java.nio.file.FileSystemException: foo.jar: The process cannot access the file because it is being used by another process " Now when the same code is run (on Windows) *with* the proposed FILE_SHARE_DELETE changes, I notice that line#2 returns "true" (i.e. the file is considered deleted) and line#3 returns "true" (i.e. it's considered the file still exists). So essentially the file is only marked for deletion and hasn't been deleted at the OS level, since the JarFile instance still has an handle open. Then line#4 unlike previously, now throws an exception and fails to create the FileOutputStream instance. The failure says "java.nio.file.AccessDeniedException: foo.jar". This exception matches the documentation from that Windows DeleteFile API. line#5 too fails with this same AccessDeniedException. So what this means is, although we have managed to allow the deletion to happen (i.e. the first call to delete does mark it for deletion at OS level), IMO, it still doesn't improve much and raises other difference in behaviour of APIs as seen above, unless the underlying file handle held by ZipFile/JarFile is closed. So that brings me to the next related issue. For this change to be really useful, the file handle (which is an instance of RandomAccessFile) held by ZipFile/JarFile needs to be closed. If a user application explicitly creates an instance of ZipFile/JarFile, it is expected that they close it themselves. However, the most prominent use of the JarFile creation is within the JDK code where it uses the sun.net.www.protocol.jar.JarURLConnection to construct the JarFile instances, typically from the URLClassLoader. By default these JarFile instances that are created by the sun.net.www.protocol.jar.JarURLConnection are cached and the close() on those instances is called only when the URLClassLoader is close()ed. That in itself doesn't guarantee that the underlying file descriptor corresponding to the JarFile instance gets closed, because there's one more level of reference counting "cache" in use in java.util.zip.ZipFile$Source class where the same underlying file descriptor (corresponding to the jar file) gets used by multiple JarFile instances (irrespective of who created those). So in short, the underlying file descriptor of a JarFile only gets clo
Re: RFR: 8275342: Change nested classes in java.prefs to static nested classes
On Fri, 15 Oct 2021 18:51:44 GMT, Andrey Turbanov wrote: > Non-static classes hold a link to their parent classes, which can be avoided. I usually don't review changes in this area of core-libs but these are simple enough and look reasonable. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5971
Integrated: JDK-8276150: Quarantined jpackage apps are labeled as "damaged"
On Mon, 15 Nov 2021 19:56:28 GMT, Andy Herrick wrote: > JDK-8276150: Quarantined jpackage apps are labeled as "damaged" This pull request has now been integrated. Changeset: 936f7ff4 Author:Andy Herrick URL: https://git.openjdk.java.net/jdk/commit/936f7ff49ed86adb74bb1ff10d93cb3d7f7d70a0 Stats: 102 lines in 2 files changed: 69 ins; 22 del; 11 mod 8276150: Quarantined jpackage apps are labeled as "damaged" Reviewed-by: almatvee - PR: https://git.openjdk.java.net/jdk/pull/6396
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod
On Thu, 18 Nov 2021 11:42:48 GMT, Magnus Ihse Bursie wrote: >>> Just to verify, this test jar had 16000 files in a single directory? Since >>> having 100 files in 160 directories would not have the same impact, right? >> >> @magicus so the first test I did was 16,000 files in multiple directories, >> as I thought that was more representative, and would involve multiple small >> sorts vs one large sort. >> I have just re-run the tests again, for both 16000 multi-dir and 16000 >> single dir, and the results are similar: >> 16000 multi-dir x50: >> Existing code: 291 seconds (variance: +/- 3 seconds) >> New patch: 293 seconds (variance: +/- 3 seconds) >> 16000 single-dir x50: >> Existing code: 235 seconds (variance: +/- 3 seconds) >> New patch: 237 seconds (variance: +/- 3 seconds) > > @andrew-m-leonard Thanks. I think that alleviates any fear of performance > regression. Sounds like you are well within the span of normal variation. @magicus @mlchung @LanceAndersen ready for review again please, with all changes incorporated. thanks - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8275342: Change nested classes in java.prefs to static nested classes
On Fri, 15 Oct 2021 18:51:44 GMT, Andrey Turbanov wrote: > Non-static classes hold a link to their parent classes, which can be avoided. LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5971
Re: RFR: 8276806: Use Objects.checkFromIndexSize where possible in java.base [v2]
On Mon, 8 Nov 2021 18:59:06 GMT, Сергей Цыпанов wrote: >> This is a follow-up for https://github.com/openjdk/jdk/pull/4507, looks like >> there are some cases that were not covered. >> >> Also this is related to https://github.com/openjdk/jdk/pull/3615 > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8276806: Use Objects.checkFromIndexSize where possible in java.base Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6297
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
LGTM
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Looks good.
-
Marked as reviewed by alanb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v5]
On Sun, 14 Nov 2021 16:45:07 GMT, Ichiroh Takiguchi
wrote:
>> Good suggestions. Filed a JBS issue:
>> https://bugs.openjdk.java.net/browse/JDK-8276970
>
> Hello @naotoj .
> For PrintStream.getCharset(), following changes may be required.
>
> +++ src/java.base/share/classes/java/io/OutputStreamWriter.java
> +Charset getCharset() {
> +return se.getCharset();
> +}
>
> +++ src/java.base/share/classes/java/io/PrintStream.java
> +public Charset getCharset() {
> +return charOut.getCharset();
> +}
>
> +++ src/java.base/share/classes/sun/nio/cs/StreamEncoder.java
> +public Charset getCharset() {
> +return cs;
> +}
>
> For javac code, we may not use PrintStream.getCharset() directly because
> javac code is compiled by boot compiler.
> We need to use reflection, like:
>
> +++ src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
> +private static Charset getCharset(PrintStream ps) {
> +try {
> +Method getCharset =
> PrintStream.class.getDeclaredMethod("getCharset");
> +return (Charset)getCharset.invoke(ps);
> +} catch (Exception e) {
> +return Charset.defaultCharset();
> +}
> +}
>
> If we add following constructors against PrintWriter, we just change javap
> and jshell code.
> But I cannot evaluate this code changes.
>
> +++ src/java.base/share/classes/java/io/PrintWriter.java
> +public PrintWriter(PrintStream ps) {
> + this((OutputStream)ps, false, ps.getCharset());
> +}
> +public PrintWriter(PrintStream ps, boolean autoFlush) {
> + this((OutputStream)ps, autoFlush, ps.getCharset());
> +}
>
> I really appreciate if you handle this kind of code change via JEP-400.
I think this PR can now safely be withdrawn, as
https://github.com/openjdk/jdk/pull/6401 is now integrated. @takiguc, if you do
not mind, I will create a PR for the remaining jshell issue. Please let me know.
-
PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: 8276141: XPathFactory set/getProperty method [v10]
On Thu, 18 Nov 2021 23:43:20 GMT, Joe Wang wrote: >> Add setProperty/getProperty methods to the XPath API so that properties can >> be supported in the future. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > update as commented Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6215
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 10:10:33 GMT, Andrew Leonard wrote:
>> Both jar and jmod utilise java.io file operations whose methods define no
>> ordering of the return file lists, and in fact rely on OS query file
>> ordering, which can differ by underlying OS architecture.
>> This PR adds sort processing to the creation of such jar's and jmod's to
>> enable a deterministic content ordering.
>>
>> Signed-off-by: Andrew Leonard
>
> Andrew Leonard has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8276764: Enable deterministic file content ordering for Jar and Jmod
>
> Signed-off-by: Andrew Leonard
The source change looks good. I have a couple comments for the tests.
test/jdk/tools/jar/ContentOrder.java line 55:
> 53: import jdk.test.lib.util.FileUtils;
> 54:
> 55: public class ContentOrder {
It'd be useful to include the test cases to verify the `--release` option with
the specified files and directories as well.
test/jdk/tools/jar/ContentOrder.java line 89:
> 87: jar("cf test.jar testjar");
> 88: jar("tf test.jar");
> 89: println();
Suggestion:
System.out.println(new String(baos.toByteArray()));
You can simply print the output from `jar tf` command here and the `println`
method isn't really needed.
test/jdk/tools/jar/CreateJarBenchmark.java line 56:
> 54: import jdk.test.lib.util.FileUtils;
> 55:
> 56: public class CreateJarBenchmark {
This is a benchmark which should not be run as regression testing. I wonder
if we need this benchmark for future use. If so I would suggest to convert it
to JMH and place it under `test/micro/org/openjdk/bench/tools/jar` directory.
I'm okay with or without it in this patch.
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276141: XPathFactory set/getProperty method [v10]
On Thu, 18 Nov 2021 23:43:20 GMT, Joe Wang wrote: >> Add setProperty/getProperty methods to the XPath API so that properties can >> be supported in the future. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > update as commented Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6215
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by lancea (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
RFR: 8272042: java.util.ImmutableCollections$Map1 and MapN should not be @ValueBased
The `jdk.internal.ValueBased` annotation was incorrectly applied to subclasses of java.util.AbstractMap. [ValueBased](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/doc-files/ValueBased.html) requires that supertypes have no instance fields; AbstractMap has instance fields keySet and values. Remove the internal @ValueBased annotation for subclasses of AbstractMap including: AbstractImmutableMap, Map1, and MapN. - Commit messages: - 8272042: java.util.ImmutableCollections$Map1 and MapN should not be @ValueBased Changes: https://git.openjdk.java.net/jdk/pull/6480/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6480&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272042 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6480.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6480/head:pull/6480 PR: https://git.openjdk.java.net/jdk/pull/6480
Re: RFR: 8272042: java.util.ImmutableCollections$Map1 and MapN should not be @ValueBased
On Fri, 19 Nov 2021 15:50:47 GMT, Roger Riggs wrote: > The `jdk.internal.ValueBased` annotation was incorrectly applied to > subclasses of java.util.AbstractMap. > [ValueBased](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/doc-files/ValueBased.html) > requires that supertypes have no instance fields; AbstractMap has instance > fields keySet and values. > > Remove the internal @ValueBased annotation for subclasses of AbstractMap > including: > AbstractImmutableMap, Map1, and MapN. It may be helpful to add a comment why these classes are not value-based classes. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6480
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
src/java.base/share/classes/java/lang/Object.java line 481:
> 479: * system resources or to perform other cleanup.
> 480: *
> 481: * When running in a Java virtual machine in which finalization
> has been
Disabling finalization is not part of the Java SE spec. This paragraph
describes the implementation of HotSpot VM and I think it does not belong to
this javadoc.
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by darcy (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 17:24:15 GMT, Mandy Chung wrote:
>> Andrew Leonard has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8276764: Enable deterministic file content ordering for Jar and Jmod
>>
>> Signed-off-by: Andrew Leonard
>
> test/jdk/tools/jar/CreateJarBenchmark.java line 56:
>
>> 54: import jdk.test.lib.util.FileUtils;
>> 55:
>> 56: public class CreateJarBenchmark {
>
> This is a benchmark which should not be run as regression testing. I wonder
> if we need this benchmark for future use. If so I would suggest to convert
> it to JMH and place it under `test/micro/org/openjdk/bench/tools/jar`
> directory. I'm okay with or without it in this patch.
@LanceAndersen what's your opinion? I'm not totally sure of it's usefulness?
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Fri, 19 Nov 2021 18:06:39 GMT, Mandy Chung wrote:
>> Here are the code changes for the "Deprecate finalizers in the standard Java
>> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
>> review.
>>
>> This change makes the indicated deprecations, and updates the API spec for
>> JEP 421. It also updates the relevant @SuppressWarning annotations.
>>
>> The CSR has been approved.
>> An automated test build+test run passes cleanly (FWIW :D ).
>
> src/java.base/share/classes/java/lang/Object.java line 481:
>
>> 479: * system resources or to perform other cleanup.
>> 480: *
>> 481: * When running in a Java virtual machine in which finalization
>> has been
>
> Disabling finalization is not part of the Java SE spec. This paragraph
> describes the implementation of HotSpot VM and I think it does not belong to
> this javadoc.
The coupling between this change and
[8276422](https://bugs.openjdk.java.net/browse/JDK-8276422) ("Add command-line
option to disable finalization") is probably tighter than would be ideal. That
[CSR](https://bugs.openjdk.java.net/browse/JDK-8276773) allows for finalization
to be disabled in the SE Platform Spec and the JLS.
-
PR: https://git.openjdk.java.net/jdk/pull/6465
RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content
Add a new --source-date (epoch milliseconds) option to jar and jmod to allow specification of time to use for created/updated jar/jmod entries. This then allows the ability to make the content deterministic. Signed-off-by: Andrew Leonard - Commit messages: - 8276766: Enable jar and jmod to produce deterministic timestamped content - 8276766: Enable jar and jmod to produce deterministic timestamped content Changes: https://git.openjdk.java.net/jdk/pull/6481/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6481&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276766 Stats: 169 lines in 10 files changed: 141 ins; 0 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/6481.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6481/head:pull/6481 PR: https://git.openjdk.java.net/jdk/pull/6481
Re: RFR: 8272042: java.util.ImmutableCollections$Map1 and MapN should not be @ValueBased
On Fri, 19 Nov 2021 15:50:47 GMT, Roger Riggs wrote: > The `jdk.internal.ValueBased` annotation was incorrectly applied to > subclasses of java.util.AbstractMap. > [ValueBased](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/doc-files/ValueBased.html) > requires that supertypes have no instance fields; AbstractMap has instance > fields keySet and values. > > Remove the internal @ValueBased annotation for subclasses of AbstractMap > including: > AbstractImmutableMap, Map1, and MapN. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6480
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v4]
On Fri, 5 Nov 2021 13:52:49 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - use testng asserts > - Remove "final" usage from test Hi Jaikiran, I am OK with moving forward. You might give it a couple more days before you push in the event we get additional feedback (realizing the PR has been open for a while) Thank you for your efforts and patience on this. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 10:10:33 GMT, Andrew Leonard wrote: >> Both jar and jmod utilise java.io file operations whose methods define no >> ordering of the return file lists, and in fact rely on OS query file >> ordering, which can differ by underlying OS architecture. >> This PR adds sort processing to the creation of such jar's and jmod's to >> enable a deterministic content ordering. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > 8276764: Enable deterministic file content ordering for Jar and Jmod > > Signed-off-by: Andrew Leonard The changes look OK. Have you run the JCK tests in addition to the Mach5 tests for the areas being changed to make sure we did not trip over a conformance test? - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 18:30:17 GMT, Lance Andersen wrote: > The changes look OK. > > Have you run the JCK tests in addition to the Mach5 tests for the areas being > changed to make sure we did not trip over a conformance test? Sorry wrong PR window please ignore this comment - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 18:13:47 GMT, Andrew Leonard wrote:
>> test/jdk/tools/jar/CreateJarBenchmark.java line 56:
>>
>>> 54: import jdk.test.lib.util.FileUtils;
>>> 55:
>>> 56: public class CreateJarBenchmark {
>>
>> This is a benchmark which should not be run as regression testing. I
>> wonder if we need this benchmark for future use. If so I would suggest to
>> convert it to JMH and place it under
>> `test/micro/org/openjdk/bench/tools/jar` directory. I'm okay with or
>> without it in this patch.
>
> @LanceAndersen what's your opinion? I'm not totally sure of it's usefulness?
My thought is to remove it from this PR, since we've already determined the
change has little impact.
We can raise a new issue if we feel it's needed.
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 18:30:37 GMT, Andrew Leonard wrote: >> @LanceAndersen what's your opinion? I'm not totally sure of it's usefulness? > > My thought is to remove it from this PR, since we've already determined the > change has little impact. > We can raise a new issue if we feel it's needed. The benchmark should use JMH. @cl4es, what are your thoughts here? - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 10:10:33 GMT, Andrew Leonard wrote:
>> Both jar and jmod utilise java.io file operations whose methods define no
>> ordering of the return file lists, and in fact rely on OS query file
>> ordering, which can differ by underlying OS architecture.
>> This PR adds sort processing to the creation of such jar's and jmod's to
>> enable a deterministic content ordering.
>>
>> Signed-off-by: Andrew Leonard
>
> Andrew Leonard has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8276764: Enable deterministic file content ordering for Jar and Jmod
>
> Signed-off-by: Andrew Leonard
test/jdk/tools/jar/ContentOrder.java line 87:
> 85: onCompletion = () -> rm("test.jar", "testjar");
> 86:
> 87: jar("cf test.jar testjar");
It might be worth testing with -C dir as well
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276141: XPathFactory set/getProperty method [v10]
On Thu, 18 Nov 2021 23:43:20 GMT, Joe Wang wrote: >> Add setProperty/getProperty methods to the XPath API so that properties can >> be supported in the future. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > update as commented Associated CSR also Reviewed. - Marked as reviewed by iris (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6215
Integrated: 8274949: Use String.contains() instead of String.indexOf() in java.base
On Fri, 17 Sep 2021 08:56:47 GMT, Andrey Turbanov wrote: > String.contains was introduced in Java 5. > Some code in java.base still uses old approach with `String.indexOf` to check > if String contains specified substring. > I propose to migrate such usages. Makes code shorter and easier to read. This pull request has now been integrated. Changeset: 66775543 Author:Andrey Turbanov Committer: Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/6677554374ade32c9f2ddaaa093064de8aebd831 Stats: 38 lines in 17 files changed: 0 ins; 3 del; 35 mod 8274949: Use String.contains() instead of String.indexOf() in java.base Reviewed-by: weijun, dfuchs, vtewari, lancea - PR: https://git.openjdk.java.net/jdk/pull/5559
Integrated: 8274333: Redundant null comparison after Pattern.split
On Sun, 26 Sep 2021 15:10:52 GMT, Andrey Turbanov wrote: > In couple of classes, result part of arrays of Pattern.split is compared with > `null`. Pattern.split (and hence String.split) never returns `null` in array > elements. Such comparisons are redundant. This pull request has now been integrated. Changeset: 36b59f38 Author:Andrey Turbanov Committer: Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/36b59f3814fdaa44c9c04a0e8d63d0ba56929326 Stats: 14 lines in 2 files changed: 4 ins; 5 del; 5 mod 8274333: Redundant null comparison after Pattern.split Reviewed-by: mullan, weijun, rriggs, iris - PR: https://git.openjdk.java.net/jdk/pull/5708
Integrated: 8274946: Cleanup unnecessary calls to Throwable.initCause() in java.rmi
On Thu, 7 Oct 2021 18:00:33 GMT, Andrey Turbanov wrote: > Pass cause exception as constructor parameter is shorter and easier to read. This pull request has now been integrated. Changeset: f609b8fd Author:Andrey Turbanov Committer: Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/f609b8fd74c55f3149d8e5a6a9a9bc3598c0b961 Stats: 20 lines in 5 files changed: 0 ins; 10 del; 10 mod 8274946: Cleanup unnecessary calls to Throwable.initCause() in java.rmi Reviewed-by: iris, rriggs - PR: https://git.openjdk.java.net/jdk/pull/5854
Integrated: 8275386: Change nested classes in jdk.jlink to static nested classes
On Sun, 17 Oct 2021 21:06:44 GMT, Andrey Turbanov wrote: > Non-static classes hold a link to their parent classes, which can be avoided. This pull request has now been integrated. Changeset: e47cc81b Author:Andrey Turbanov Committer: Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/e47cc81b095381266104e9137495e91fb4c225a4 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod 8275386: Change nested classes in jdk.jlink to static nested classes Reviewed-by: alanb, rriggs, iris - PR: https://git.openjdk.java.net/jdk/pull/5983
Re: RFR: 8276806: Use Objects.checkFromIndexSize where possible in java.base [v2]
On Mon, 8 Nov 2021 18:59:06 GMT, Сергей Цыпанов wrote: >> This is a follow-up for https://github.com/openjdk/jdk/pull/4507, looks like >> there are some cases that were not covered. >> >> Also this is related to https://github.com/openjdk/jdk/pull/3615 > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8276806: Use Objects.checkFromIndexSize where possible in java.base This Looks OK to me - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6297
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 18:33:31 GMT, Lance Andersen wrote: >> My thought is to remove it from this PR, since we've already determined the >> change has little impact. >> We can raise a new issue if we feel it's needed. > > The benchmark should use JMH. @cl4es, what are your thoughts here? > My thought is to remove it from this PR, since we've already determined the > change has little impact. We can raise a new issue if we feel it's needed. This is fine with me. - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by mchung (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Fri, 19 Nov 2021 18:15:46 GMT, Brent Christian wrote:
>> src/java.base/share/classes/java/lang/Object.java line 481:
>>
>>> 479: * system resources or to perform other cleanup.
>>> 480: *
>>> 481: * When running in a Java virtual machine in which finalization
>>> has been
>>
>> Disabling finalization is not part of the Java SE spec. This paragraph
>> describes the implementation of HotSpot VM and I think it does not belong to
>> this javadoc.
>
> The coupling between this change and
> [8276422](https://bugs.openjdk.java.net/browse/JDK-8276422) ("Add
> command-line option to disable finalization") is probably tighter than would
> be ideal. That [CSR](https://bugs.openjdk.java.net/browse/JDK-8276773)
> allows for finalization to be disabled in the SE Platform Spec and the JLS.
Thanks for pointing out that the CSR for JDK-8276422 ("Add command-line option
to disable finalization") includes the change of the SE Platform Spec and JLS
to allow an implementation for finalization to be disabled. This makes senses
now.
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 10:10:33 GMT, Andrew Leonard wrote:
>> Both jar and jmod utilise java.io file operations whose methods define no
>> ordering of the return file lists, and in fact rely on OS query file
>> ordering, which can differ by underlying OS architecture.
>> This PR adds sort processing to the creation of such jar's and jmod's to
>> enable a deterministic content ordering.
>>
>> Signed-off-by: Andrew Leonard
>
> Andrew Leonard has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8276764: Enable deterministic file content ordering for Jar and Jmod
>
> Signed-off-by: Andrew Leonard
src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 812:
> 810: if (uflag) {
> 811: entryMap.put(name, e);
> 812: }
Thank you for cleaning up the if missing braces in this method. Looks much
better :-)
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 18:55:40 GMT, Mandy Chung wrote: >> The benchmark should use JMH. @cl4es, what are your thoughts here? > >> My thought is to remove it from this PR, since we've already determined the >> change has little impact. > We can raise a new issue if we feel it's needed. > > This is fine with me. I am Ok with separating this out as well but I think it would be nice to give Claes a chance to state his preference - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 17:27:45 GMT, Mandy Chung wrote:
>> Andrew Leonard has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8276764: Enable deterministic file content ordering for Jar and Jmod
>>
>> Signed-off-by: Andrew Leonard
>
> The source change looks good. I have a couple comments for the tests.
@mlchung Added -C and --release to tests, looks more complete now, thanks
> test/jdk/tools/jar/ContentOrder.java line 55:
>
>> 53: import jdk.test.lib.util.FileUtils;
>> 54:
>> 55: public class ContentOrder {
>
> It'd be useful to include the test cases to verify the `--release` option
> with the specified files and directories as well.
added
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 18:38:25 GMT, Lance Andersen wrote:
>> Andrew Leonard has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8276764: Enable deterministic file content ordering for Jar and Jmod
>>
>> Signed-off-by: Andrew Leonard
>
> test/jdk/tools/jar/ContentOrder.java line 87:
>
>> 85: onCompletion = () -> rm("test.jar", "testjar");
>> 86:
>> 87: jar("cf test.jar testjar");
>
> It might be worth testing with -C dir as well
added
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: JDK-8276422 Add command-line option to disable finalization [v4]
On Fri, 19 Nov 2021 00:14:18 GMT, Stuart Marks wrote: >> Pretty much what it says. The new option controls a static member in >> InstanceKlass that's consulted to determine whether the finalization >> machinery is activated for instances when a class is loaded. A new native >> method is added so that this state can be queried from Java. This is used to >> control whether a finalizer thread is created and to disable the `System` >> and `Runtime::runFinalization` methods. Includes tests for the above. >> >> Adding an option to disable finalization is part of [JEP >> 421](https://openjdk.java.net/jeps/421). > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Remove Finalizer.Holder class. Regarding **jcmd** updates, I'm thinking maybe this would be better handled separately. There is the potential to update to `GC.finalizer_info` discussed previously. Looking at the **jcmd** tool docs, it seems like `GC.run_finalization` also ought to be updated. And maybe one or more of the other commands (maybe `VM.flags` or `VM.info`?) ought to list the finalization enabled or disabled status. And of course the tool's doc will need to be updated as well. - PR: https://git.openjdk.java.net/jdk/pull/6442
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by serb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v4]
> Both jar and jmod utilise java.io file operations whose methods define no > ordering of the return file lists, and in fact rely on OS query file > ordering, which can differ by underlying OS architecture. > This PR adds sort processing to the creation of such jar's and jmod's to > enable a deterministic content ordering. > > Signed-off-by: Andrew Leonard Andrew Leonard has updated the pull request incrementally with one additional commit since the last revision: 8276764: Enable deterministic file content ordering for Jar and Jmod Signed-off-by: Andrew Leonard - Changes: - all: https://git.openjdk.java.net/jdk/pull/6395/files - new: https://git.openjdk.java.net/jdk/pull/6395/files/c8d6e90d..23efb28c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=02-03 Stats: 235 lines in 2 files changed: 40 ins; 193 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6395.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6395/head:pull/6395 PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: JDK-8276422 Add command-line option to disable finalization [v4]
On Fri, 19 Nov 2021 00:14:18 GMT, Stuart Marks wrote: >> Pretty much what it says. The new option controls a static member in >> InstanceKlass that's consulted to determine whether the finalization >> machinery is activated for instances when a class is loaded. A new native >> method is added so that this state can be queried from Java. This is used to >> control whether a finalizer thread is created and to disable the `System` >> and `Runtime::runFinalization` methods. Includes tests for the above. >> >> Adding an option to disable finalization is part of [JEP >> 421](https://openjdk.java.net/jeps/421). > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Remove Finalizer.Holder class. Lib changes and tests look good - Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6442
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v4]
On Fri, 19 Nov 2021 20:15:54 GMT, Andrew Leonard wrote: >> Both jar and jmod utilise java.io file operations whose methods define no >> ordering of the return file lists, and in fact rely on OS query file >> ordering, which can differ by underlying OS architecture. >> This PR adds sort processing to the creation of such jar's and jmod's to >> enable a deterministic content ordering. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > 8276764: Enable deterministic file content ordering for Jar and Jmod > > Signed-off-by: Andrew Leonard The changes look good. Could you please add a comment overviewing each test. Thank you - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 17:11:24 GMT, Mandy Chung wrote:
>> Andrew Leonard has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8276764: Enable deterministic file content ordering for Jar and Jmod
>>
>> Signed-off-by: Andrew Leonard
>
> test/jdk/tools/jar/ContentOrder.java line 89:
>
>> 87: jar("cf test.jar testjar");
>> 88: jar("tf test.jar");
>> 89: println();
>
> Suggestion:
>
> System.out.println(new String(baos.toByteArray()));
>
>
> You can simply print the output from `jar tf` command here and the `println`
> method isn't really needed.
done
-
PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v3]
On Fri, 19 Nov 2021 19:08:48 GMT, Lance Andersen wrote: >>> My thought is to remove it from this PR, since we've already determined the >>> change has little impact. >> We can raise a new issue if we feel it's needed. >> >> This is fine with me. > > I am Ok with separating this out as well but I think it would be nice to give > Claes a chance to state his preference Removed benchmark - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8272042: java.util.ImmutableCollections$Map1 and MapN should not be @ValueBased [v2]
> The `jdk.internal.ValueBased` annotation was incorrectly applied to > subclasses of java.util.AbstractMap. > [ValueBased](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/doc-files/ValueBased.html) > requires that supertypes have no instance fields; AbstractMap has instance > fields keySet and values. > > Remove the internal @ValueBased annotation for subclasses of AbstractMap > including: > AbstractImmutableMap, Map1, and MapN. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Added comment explaining why immutable Maps are not 'ValueBased' - Changes: - all: https://git.openjdk.java.net/jdk/pull/6480/files - new: https://git.openjdk.java.net/jdk/pull/6480/files/c1f579ea..42fce039 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6480&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6480&range=00-01 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6480.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6480/head:pull/6480 PR: https://git.openjdk.java.net/jdk/pull/6480
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
On Wed, 17 Nov 2021 10:34:52 GMT, Sergey Bylokhov wrote: >>> Yes it would be nice to clarify that a null is accepted by setComment. When >>> null is specified, the comment length is written as 0 >> >> @mrserb Are you taking the spec clarification or should we just created >> another issue in JBS to track it? > > I would like to update the spec for the null value for this method, and > probably others in a separate CR, since this fix could be backported to the > early releases. Will create such CR after agreement. I have filed a separate issue: https://bugs.openjdk.java.net/browse/JDK-8277495 - PR: https://git.openjdk.java.net/jdk/pull/6380
RFR: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
This reverts commit 936f7ff49ed86adb74bb1ff10d93cb3d7f7d70a0. So far we've had 3 failed Tier2 job sets in a row. My Mach5 Tier2 of this [BACKOUT] has passed the macosx-aarch64 test task that was failing before. - Commit messages: - Revert "8276150: Quarantined jpackage apps are labeled as "damaged"" Changes: https://git.openjdk.java.net/jdk/pull/6483/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6483&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277494 Stats: 102 lines in 2 files changed: 22 ins; 69 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/6483.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6483/head:pull/6483 PR: https://git.openjdk.java.net/jdk/pull/6483
Re: RFR: 8272042: java.util.ImmutableCollections$Map1 and MapN should not be @ValueBased [v2]
On Fri, 19 Nov 2021 20:47:40 GMT, Roger Riggs wrote: >> The `jdk.internal.ValueBased` annotation was incorrectly applied to >> subclasses of java.util.AbstractMap. >> [ValueBased](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/doc-files/ValueBased.html) >> requires that supertypes have no instance fields; AbstractMap has instance >> fields keySet and values. >> >> Remove the internal @ValueBased annotation for subclasses of AbstractMap >> including: >> AbstractImmutableMap, Map1, and MapN. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Added comment explaining why immutable Maps are not 'ValueBased' Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6480
Re: RFR: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
On Fri, 19 Nov 2021 20:57:46 GMT, Daniel D. Daugherty wrote: > This reverts commit 936f7ff49ed86adb74bb1ff10d93cb3d7f7d70a0. > > So far we've had 3 failed Tier2 job sets in a row. My Mach5 Tier2 of this > [BACKOUT] has > passed the macosx-aarch64 test task that was failing before. My Mach5 Tier2 of this [BACKOUT] has passed all tests on all platforms. - PR: https://git.openjdk.java.net/jdk/pull/6483
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v5]
> Both jar and jmod utilise java.io file operations whose methods define no > ordering of the return file lists, and in fact rely on OS query file > ordering, which can differ by underlying OS architecture. > This PR adds sort processing to the creation of such jar's and jmod's to > enable a deterministic content ordering. > > Signed-off-by: Andrew Leonard Andrew Leonard has updated the pull request incrementally with one additional commit since the last revision: 8276764: Enable deterministic file content ordering for Jar and Jmod Signed-off-by: Andrew Leonard - Changes: - all: https://git.openjdk.java.net/jdk/pull/6395/files - new: https://git.openjdk.java.net/jdk/pull/6395/files/23efb28c..18776eb3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=03-04 Stats: 18 lines in 3 files changed: 18 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6395.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6395/head:pull/6395 PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v4]
On Fri, 19 Nov 2021 20:25:56 GMT, Lance Andersen wrote: > The changes look good. > > Could you please add a comment overviewing each test. Thank you Done - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod
On Thu, 18 Nov 2021 11:42:48 GMT, Magnus Ihse Bursie wrote: >>> Just to verify, this test jar had 16000 files in a single directory? Since >>> having 100 files in 160 directories would not have the same impact, right? >> >> @magicus so the first test I did was 16,000 files in multiple directories, >> as I thought that was more representative, and would involve multiple small >> sorts vs one large sort. >> I have just re-run the tests again, for both 16000 multi-dir and 16000 >> single dir, and the results are similar: >> 16000 multi-dir x50: >> Existing code: 291 seconds (variance: +/- 3 seconds) >> New patch: 293 seconds (variance: +/- 3 seconds) >> 16000 single-dir x50: >> Existing code: 235 seconds (variance: +/- 3 seconds) >> New patch: 237 seconds (variance: +/- 3 seconds) > > @andrew-m-leonard Thanks. I think that alleviates any fear of performance > regression. Sounds like you are well within the span of normal variation. @magicus I've added updates to the jar and jmod man pages, as "Notes" about ordering, does that sound reasonable? - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
On Fri, 19 Nov 2021 20:57:46 GMT, Daniel D. Daugherty wrote: > This reverts commit 936f7ff49ed86adb74bb1ff10d93cb3d7f7d70a0. > > So far we've had 3 failed Tier2 job sets in a row. My Mach5 Tier2 of this > [BACKOUT] has > passed the macosx-aarch64 test task that was failing before. My Mach5 Tier2 of the baseline(master) with the fix for JDK-8276150 has reproduced the 31 failures on macosx-aarch64. So that's now 4 Tier2 job sets that have seen the failures. - PR: https://git.openjdk.java.net/jdk/pull/6483
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod
On Fri, 19 Nov 2021 21:22:28 GMT, Andrew Leonard wrote: > I've added updates to the jar and jmod man pages, as "Notes" about ordering, > does that sound reasonable? The true source of the man pages are still kept in our closed repository. The copy in the jdk repo is generated from our closed source and updated near the end of each release.We should create a follow-up issue to update the man pages that perhaps Lance can help updating that. - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod
On Fri, 19 Nov 2021 21:33:43 GMT, Mandy Chung wrote: >> @magicus I've added updates to the jar and jmod man pages, as "Notes" about >> ordering, does that sound reasonable? > >> I've added updates to the jar and jmod man pages, as "Notes" about >> ordering, does that sound reasonable? > > The true source of the man pages are still kept in our closed repository. > The copy in the jdk repo is generated from our closed source and updated near > the end of each release.We should create a follow-up issue to update the > man pages that perhaps Lance can help updating that. @mlchung ah thanks Mandy, I didn't realize that. I'll remove from here and create a follow up once this is merged, thanks - PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v6]
> Both jar and jmod utilise java.io file operations whose methods define no > ordering of the return file lists, and in fact rely on OS query file > ordering, which can differ by underlying OS architecture. > This PR adds sort processing to the creation of such jar's and jmod's to > enable a deterministic content ordering. > > Signed-off-by: Andrew Leonard Andrew Leonard has updated the pull request incrementally with one additional commit since the last revision: 8276764: Enable deterministic file content ordering for Jar and Jmod Signed-off-by: Andrew Leonard - Changes: - all: https://git.openjdk.java.net/jdk/pull/6395/files - new: https://git.openjdk.java.net/jdk/pull/6395/files/18776eb3..2a7668f1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6395&range=04-05 Stats: 14 lines in 2 files changed: 0 ins; 14 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6395.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6395/head:pull/6395 PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
On Fri, 19 Nov 2021 20:57:46 GMT, Daniel D. Daugherty wrote: > This reverts commit 936f7ff49ed86adb74bb1ff10d93cb3d7f7d70a0. > > So far we've had 3 failed Tier2 job sets in a row. My Mach5 Tier2 of this > [BACKOUT] has > passed the macosx-aarch64 test task that was failing before. Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6483
Re: RFR: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
On Fri, 19 Nov 2021 20:57:46 GMT, Daniel D. Daugherty wrote: > This reverts commit 936f7ff49ed86adb74bb1ff10d93cb3d7f7d70a0. > > So far we've had 3 failed Tier2 job sets in a row. My Mach5 Tier2 of this > [BACKOUT] has > passed the macosx-aarch64 test task that was failing before. Marked as reviewed by tschatzl (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6483
Re: RFR: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
On Fri, 19 Nov 2021 22:33:40 GMT, Alexey Semenyuk wrote: >> This reverts commit 936f7ff49ed86adb74bb1ff10d93cb3d7f7d70a0. >> >> So far we've had 3 failed Tier2 job sets in a row. My Mach5 Tier2 of this >> [BACKOUT] has >> passed the macosx-aarch64 test task that was failing before. > > Marked as reviewed by asemenyuk (Reviewer). @alexeysemenyukoracle and @tschatzl - Thanks for the reviews! - PR: https://git.openjdk.java.net/jdk/pull/6483
Integrated: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
On Fri, 19 Nov 2021 20:57:46 GMT, Daniel D. Daugherty wrote: > This reverts commit 936f7ff49ed86adb74bb1ff10d93cb3d7f7d70a0. > > So far we've had 3 failed Tier2 job sets in a row. My Mach5 Tier2 of this > [BACKOUT] has > passed the macosx-aarch64 test task that was failing before. This pull request has now been integrated. Changeset: c79a485f Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/c79a485f1c3f9c0c3a79b8847fdcd50a141cd529 Stats: 102 lines in 2 files changed: 22 ins; 69 del; 11 mod 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged" Reviewed-by: asemenyuk, tschatzl - PR: https://git.openjdk.java.net/jdk/pull/6483
Re: RFR: JDK-8276422 Add command-line option to disable finalization [v4]
On Fri, 19 Nov 2021 20:13:06 GMT, Stuart Marks wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove Finalizer.Holder class. > > Regarding **jcmd** updates, I'm thinking maybe this would be better handled > separately. There is the potential to update to `GC.finalizer_info` discussed > previously. Looking at the **jcmd** tool docs, it seems like > `GC.run_finalization` also ought to be updated. And maybe one or more of the > other commands (maybe `VM.flags` or `VM.info`?) ought to list the > finalization enabled or disabled status. And of course the tool's doc will > need to be updated as well. @stuart-marks no issue with doing dcmd/jcmd changes separately, but I don't think we need to go too far with this. I had considered `GC.run_finalization` but it just says it calls `System.run_finalization` - so no change needed there as it will be documented in System.runFinalization. And `VM.flags` only reports `-XX` flag information. And `VM.info` doesn't seem appropriate for mentioning this either. So no further changes needed to the other Dcmds IMO and no need to update anything on the jcmd tool page either. - PR: https://git.openjdk.java.net/jdk/pull/6442
Re: RFR: 8276216: Negated character classes performance regression in Pattern
On Wed, 17 Nov 2021 18:48:14 GMT, Volker Simonis wrote: > In JDK 9, function objects implemented as anonymous inner classes in > java.util.regex.Pattern have been replaced by lambda functions (see > [JDK-8151481](https://bugs.openjdk.java.net/browse/JDK-8151481)). This led to > a significant performance regression (up to ~8X) for patterns with negated > character classes (e.g. "[^A-Za-z0-9]"). > > JDK 8 > - > Benchmark (patternString) > (text) Mode CntScore Error Units > FindPattern.testFind [^A-Za-z0-9] > abcdefghijklmnop1234567890ABCDEFGHIJKLMNOP avgt3 241.359 ± 142.079 > ns/op > FindPattern.testFind [^A-Za-z0-9] > ,, avgt3 754.492 ± 181.505 > ns/op > FindPattern.testFind [A-Za-z0-9] > abcdefghijklmnop1234567890ABCDEFGHIJKLMNOP avgt3 738.998 ± 402.188 > ns/op > FindPattern.testFind [A-Za-z0-9] > ,, avgt3 260.550 ± 118.717 > ns/op > > JDK 17 > -- > Benchmark (patternString) > (text) Mode Cnt Score Error Units > FindPattern.testFind [^A-Za-z0-9] > abcdefghijklmnop1234567890ABCDEFGHIJKLMNOP avgt3 1921.954 ± 90.698 > ns/op > FindPattern.testFind [^A-Za-z0-9] > ,, avgt3 2496.115 ± 555.147 > ns/op > FindPattern.testFind [A-Za-z0-9] > abcdefghijklmnop1234567890ABCDEFGHIJKLMNOP avgt3 873.154 ± 178.640 > ns/op > FindPattern.testFind [A-Za-z0-9] > ,, avgt3 349.939 ± 33.585 > ns/op > > JDK 17 fixed > --- > Benchmark (patternString) > (text) Mode CntScore Error Units > FindPattern.testFind [^A-Za-z0-9] > abcdefghijklmnop1234567890ABCDEFGHIJKLMNOP avgt3 343.541 ± 181.147 > ns/op > FindPattern.testFind [^A-Za-z0-9] > ,, avgt3 744.226 ± 114.802 > ns/op > FindPattern.testFind [A-Za-z0-9] > abcdefghijklmnop1234567890ABCDEFGHIJKLMNOP avgt3 753.297 ± 216.331 > ns/op > FindPattern.testFind [A-Za-z0-9] > ,, avgt3 240.142 ± 13.339 > ns/op > > > ## TL;DR > > The reason for this regression is trashing of the `_secondary_super_cache` > entry in the generated Lambda classes. During matching we alternately call > methods from different interfaces on the generated Lambda class objects with > the effect that the quick type check against the `_secondary_super_cache` > continuously fails. Consequently we have to take the slow path which does a > linear search over all secondary super types and updates > `_secondary_super_cache`. > > This was the "short" description but as I've personally found the details > quite intricate I've wrote a much more detailed description of the problem at > the end of this PR. > > The fix of the problem is quite simple. Move the Lambda-generating methods > out of the function interfaces. As a result the calls to the lambda functions > become static and don't depend on the exact type of the Lambda classes any > more. > > I've also removed the overloaded `union(CharPredicate... predicates)` version > from `BmpCharPredicate`. First, it is not used anywhere anyway but second and > more important, its implementation is plain wrong. You can't cast a > `CharPredicate` into a `BmpCharPredicate` - this will deterministically lead > to a `ClassCastException` at runtime. That said, I think I understand the > motivation behind that overloaded version of `union(..)`. It could be use to > minimize the number of union nodes (i.e. predicates) for character classes > with more than two ranges (e.g. [a-zA-Z0-9]). But I leave that optimization > and a correct implementation of `union(CharPredicate... predicates)` for a > future change. > > ## Long, but maybe worth reading :) > > Currently, when a `Pattern` is compiled, we build a tree of `Pattern$Node`s. > Each `Node` has a `match()` method to match its part of the pattern. For > matching a character we use the general `CharProperty` node and the derived > `BmpCharProperty` node which only match characters from the [Basic > Multilingual > Plane](https://en.wikipedia.org/wiki/Plane_(Unicode)#Basic_Multilingual_Plane) > (i.e. characters which can be encoded in 16 bits). These nodes take a > `CharPredicate` or `BmpCharPredicate` argument respectively which are used to > decide if the character in question will be matched. `BmpCharPredicate` is a > specialization of and derived from `CharPredicate`. Since JDK 9 > `CharPredicate`/`BmpCharPredicate` are `@FunctionalInterface`s which are > implemented by various Lambda functions. They all impleme
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]
> The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge latest from master branch - use testng asserts - Remove "final" usage from test - convert test to testng - Merge latest from master branch - Merge latest from master branch - 8258117: jar tool sets the time stamp of module-info.class entries to the current time - Changes: - all: https://git.openjdk.java.net/jdk/pull/5486/files - new: https://git.openjdk.java.net/jdk/pull/5486/files/2c0246f9..945fde6f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5486&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5486&range=03-04 Stats: 48509 lines in 1041 files changed: 34004 ins; 7147 del; 7358 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v4]
On Fri, 5 Nov 2021 13:52:49 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - use testng asserts > - Remove "final" usage from test Hello Lance, > Hi Jaikiran, > > I am OK with moving forward. Thank you for the review. > You might give it a couple more days before you push in the event we get > additional feedback (realizing the PR has been open for a while) > > Thank you for your efforts and patience on this. Certainly. I won't integrate this until this next Tuesday. In the meantime, given that this has been open for a while now and new commits have made it to master, I will go ahead and merge the latest master changes just to be sure nothing surprisingly breaks. - PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: JDK-8276422 Add command-line option to disable finalization [v4]
On Fri, 19 Nov 2021 22:50:49 GMT, David Holmes wrote: >> Regarding **jcmd** updates, I'm thinking maybe this would be better handled >> separately. There is the potential to update to `GC.finalizer_info` >> discussed previously. Looking at the **jcmd** tool docs, it seems like >> `GC.run_finalization` also ought to be updated. And maybe one or more of the >> other commands (maybe `VM.flags` or `VM.info`?) ought to list the >> finalization enabled or disabled status. And of course the tool's doc will >> need to be updated as well. > > @stuart-marks no issue with doing dcmd/jcmd changes separately, but I don't > think we need to go too far with this. I had considered `GC.run_finalization` > but it just says it calls `System.run_finalization` - so no change needed > there as it will be documented in System.runFinalization. And `VM.flags` only > reports `-XX` flag information. And `VM.info` doesn't seem appropriate for > mentioning this either. So no further changes needed to the other Dcmds IMO > and no need to update anything on the jcmd tool page either. @dholmes-ora OK if you're confident that it's sufficient just to add `GC.finalizer_info` and nothing else, and no docs or additional testing, then I'll just drop in the code from that branch you posted. Of course I'll do a full build & test. - PR: https://git.openjdk.java.net/jdk/pull/6442
RFR: 8277429: Conflicting jpackage static library name
8277429: Conflicting jpackage static library name - Commit messages: - 8277429: Conflicting jpackage static library name Changes: https://git.openjdk.java.net/jdk/pull/6485/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6485&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277429 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6485.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6485/head:pull/6485 PR: https://git.openjdk.java.net/jdk/pull/6485
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v3]
On Fri, 19 Nov 2021 05:07:23 GMT, Vicente Romero wrote: >> Please review this PR which aims to optimize the implementation of the >> `toString` method we provide for records. A benchmark comparing the >> implementation we are providing for records with lombok found out that >> lombok is much faster mainly because our implementation uses >> `String::format`. This fix is basically delegating on >> StringConcatFactory::makeConcatWithConstants which is faster. >> >> TIA >> >> This is the result of the benchmark comparing records to lombok with vanilla >> JDK: >> >> Benchmark Mode CntScoreError Units >> MyBenchmark.base avgt40.849 ± 0.111 ns/op >> MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op >> MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op >> MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op >> MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op >><-- Before >> MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op >> MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op >> >> >> after this patch: >> >> Benchmark Mode Cnt Score Error Units >> MyBenchmark.base avgt4 0.680 ± 0.185 ns/op >> MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op >> MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op >> MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op >> MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op >><--- After >> MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op >> MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > adding the benchmark Regarding the slot limit in `StringConcatFactory`, it's not clear to me the limit of 200 is normative or is merely an implementation note. The limit of 200 slots seems to be arbitrary and shouldn't be baked into the spec. Perhaps this limit can be removed if the splitting logic is moved into `StringConcatFactory`. - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8277429: Conflicting jpackage static library name
On Sat, 20 Nov 2021 03:26:51 GMT, Alexey Semenyuk wrote: > 8277429: Conflicting jpackage static library name Marked as reviewed by almatvee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6485
