Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]
On Mon, 1 Nov 2021 22:35:58 GMT, Claes Redestad wrote: >> The commentary on this line could probably be improved, but this is in a >> private printer-parser that will only be used for NANO_OF_SECOND and not any >> arbitrary `TemporalField` (see line 704), thus I fail to see how this >> assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to >> 999,999,999). >> >> I considered writing a more generic integral-fraction printer parser that >> would optimize for any value-range that fits in an int, but seeing how >> NANO_OF_SECOND is likely the only one used in practice and with a high >> demand for better efficiency I opted to specialize for it more directly. > > I see what you're saying that an arbitrary `Temporal` could define its own > fields with its own ranges, but I would consider it a design bug if such an > implementation at a whim redefines the value ranges of well-defined constants > such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect such a > `Temporal` would have to define its own enumeration of allowed > `TemporalField`s. That isn't the design model however. The design model for the formatter is a `Map` like view of field to value. Any value may be associated with any field - that is exactly what `Temporal` offers. [`TempralAccessor.getLong()`](https://download.java.net/java/early_access/loom/docs/api/java.base/java/time/temporal/TemporalAccessor.html#getLong(java.time.temporal.TemporalField)) is very explicit about this. As indicated above, the positive part is that an hour-of-day of 26 can be printed by a user-written `WrappingLocalTime` class. The downside is the inability to make optimizing assumptions as per this code. FWIW, I had originally intended to write dedicated private formatters where the pattern and type to be formatted are known, such as `LocalDate` and the ISO pattern. - PR: https://git.openjdk.java.net/jdk/pull/6188
Withdrawn: JDK-8272192 Shortcut String equality checks by checking equality of the value array
On Sat, 4 Sep 2021 11:59:58 GMT, q2q-2q2 wrote: > Shortcut String equality checks by checking equality of the value array This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/5370
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v6]
> Hi all, > > Please review this fix for Infinite loop in ZipOutputStream.close(). > The main issue here is when ever there is an exception during close > operations on GZip we are not setting the deflator to a finished state which > is leading to an infinite loop when we try writing on the same GZip instance( > since we use while(!def.finished()) inside the write operation). > > Thanks, > Ravi Ravi Reddy has updated the pull request incrementally with one additional commit since the last revision: 8193682 : Infinite loop in ZipOutputStream.close() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/01d84462..3154b516 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5522&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5522&range=04-05 Stats: 116 lines in 2 files changed: 59 ins; 20 del; 37 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8275766: (tz) Update Timezone Data to 2021e
On Thu, 28 Oct 2021 01:02:27 GMT, Yoshiki Sato wrote: > Please review the integration of tzdata2021e (including tzdata2021d) to the > JDK. > The fix has passed all relevant JTREG regression tests and JCK tests. > > 8275754: (tz) Update Timezone Data to 2021d > 8275849: TestZoneInfo310.java fails with tzdata2021e Marked as reviewed by coffeys (Reviewer). I think you've enough Reviewers already but yes, this looks fine. - PR: https://git.openjdk.java.net/jdk/pull/6144
Integrated: JDK-8276236: Table headers missing in Formatter api docs
On Mon, 1 Nov 2021 15:59:22 GMT, Ludvig Janiuk wrote: > Adds table headers to all tables in Formatter api docs. I inferred the header > "Unicode" for the columns that contained unicode > codepoints. > > Formatter api docs: > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Formatter.html This pull request has now been integrated. Changeset: 92be9d8c Author:Ludvig Janiuk Committer: Sean Coffey URL: https://git.openjdk.java.net/jdk/commit/92be9d8c535274eea4edd06273e6d7811f6bb113 Stats: 80 lines in 1 file changed: 80 ins; 0 del; 0 mod 8276236: Table headers missing in Formatter api docs Reviewed-by: coffeys, iris - PR: https://git.openjdk.java.net/jdk/pull/6195
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v7]
> Hi all, > > Please review this fix for Infinite loop in ZipOutputStream.close(). > The main issue here is when ever there is an exception during close > operations on GZip we are not setting the deflator to a finished state which > is leading to an infinite loop when we try writing on the same GZip instance( > since we use while(!def.finished()) inside the write operation). > > Thanks, > Ravi Ravi Reddy has updated the pull request incrementally with one additional commit since the last revision: 8193682 : Infinite loop in ZipOutputStream.close() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/3154b516..bd0ccd7c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5522&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5522&range=05-06 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8252990: Intrinsify Unsafe.storeStoreFence [v2]
On Thu, 28 Oct 2021 08:58:48 GMT, Aleksey Shipilev wrote: >> `Unsafe.storeStoreFence` currently delegates to stronger >> `Unsafe.storeFence`. We can teach compilers to map this directly to already >> existing rules that handle `MemBarStoreStore`. Like explicit >> `LoadFence`/`StoreFence`, we introduce the special node to differentiate >> explicit fence and implicit store-store barriers. `storeStoreFence` is >> usually used to simulate safe `final`-field like constructions in special >> JDK classes, like `ConstantCallSite` and friends. >> >> Motivational performance difference on benchmarks from JDK-8276054 on ARM32 >> (Raspberry Pi 4): >> >> >> Benchmark Mode Cnt ScoreError Units >> Multiple.plain avgt3 2.669 ± 0.004 ns/op >> Multiple.release avgt3 16.688 ± 0.057 ns/op >> Multiple.storeStoreavgt3 14.021 ± 0.144 ns/op // Better >> >> MultipleWithLoads.plainavgt3 4.672 ± 0.053 ns/op >> MultipleWithLoads.release avgt3 16.689 ± 0.044 ns/op >> MultipleWithLoads.storeStore avgt3 14.012 ± 0.010 ns/op // Better >> >> MultipleWithStores.plain avgt3 14.687 ± 0.009 ns/op >> MultipleWithStores.release avgt3 45.393 ± 0.192 ns/op >> MultipleWithStores.storeStore avgt3 38.048 ± 0.033 ns/op // Better >> >> Publishing.plain avgt3 27.079 ± 0.201 ns/op >> Publishing.release avgt3 27.088 ± 0.241 ns/op >> Publishing.storeStore avgt3 27.009 ± 0.259 ns/op // Within >> error, hidden by allocation >> >> Single.plain avgt3 2.670 ± 0.002 ns/op >> Single.releaseFenceavgt3 6.675 ± 0.001 ns/op >> Single.storeStoreFence avgt3 8.012 ± 0.027 ns/op // Worse, >> seems to be ARM32 implementation artifact >> >> >> The same thing on AArch64 (Raspberry Pi 3): >> >> >> Benchmark Mode Cnt Score Error Units >> >> Multiple.plain avgt3 5.914 ± 0.115 ns/op >> Multiple.release avgt3 10.149 ± 0.059 ns/op >> Multiple.storeStoreavgt3 6.757 ± 0.138 ns/op // Better >> >> MultipleWithLoads.plainavgt3 11.849 ± 0.331 ns/op >> MultipleWithLoads.release avgt3 35.565 ± 1.144 ns/op >> MultipleWithLoads.storeStore avgt3 19.441 ± 0.471 ns/op // Better >> >> MultipleWithStores.plain avgt3 5.920 ± 0.213 ns/op >> MultipleWithStores.release avgt3 20.286 ± 0.347 ns/op >> MultipleWithStores.storeStore avgt3 12.686 ± 0.230 ns/op // Better >> >> Publishing.plain avgt3 22.261 ± 1.630 ns/op >> Publishing.release avgt3 22.269 ± 0.576 ns/op >> Publishing.storeStore avgt3 17.464 ± 0.397 ns/op // Better >> >> Single.plain avgt3 5.916 ± 0.063 ns/op >> Single.release avgt3 10.148 ± 0.401 ns/op >> Single.storeStore avgt3 6.767 ± 0.164 ns/op // Better >> >> >> As expected, this does not affect x86_64 at all, because both `release` and >> `storeStore` are effectively no-ops, only affecting compiler optimizations: >> >> >> Benchmark Mode Cnt Score Error Units >> >> Multiple.plain avgt3 0.406 ± 0.002 ns/op >> Multiple.release avgt3 0.409 ± 0.018 ns/op >> Multiple.storeStoreavgt3 0.406 ± 0.001 ns/op >> >> MultipleWithLoads.plainavgt3 4.328 ± 0.006 ns/op >> MultipleWithLoads.release avgt3 4.600 ± 0.014 ns/op >> MultipleWithLoads.storeStore avgt3 4.602 ± 0.006 ns/op >> >> MultipleWithStores.plain avgt3 0.812 ± 0.001 ns/op >> MultipleWithStores.release avgt3 0.812 ± 0.002 ns/op >> MultipleWithStores.storeStore avgt3 0.812 ± 0.002 ns/op >> >> Publishing.plain avgt3 6.370 ± 0.059 ns/op >> Publishing.release avgt3 6.358 ± 0.436 ns/op >> Publishing.storeStore avgt3 6.367 ± 0.054 ns/op >> >> Single.plain avgt3 0.407 ± 0.039 ns/op >> Single.releaseFenceavgt3 0.406 ± 0.001 ns/op >> Single.storeStoreFence avgt3 0.406 ± 0.001 ns/op >> >> >> Additional testing: >> - [x] Linux x86_64 fastdebug `tier1` >> - [x] Linux AArch64 fastdebug `tier1` >> - [x] Linux x86_64 Fences benchmark >> - [x] Linux AArch64 Fences benchmark >> - [x] Linux ARM32 Fences benchmark >> - [x] Linux AArch64 jcstress `quick` run > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Fix the comment to match JDK-8276096 jcstress and tier1 passes on AArch64. Seems like we are good to go. - PR: https://git.openjdk.java.net/jdk/pull/6136
Integrated: 8252990: Intrinsify Unsafe.storeStoreFence
On Wed, 27 Oct 2021 11:53:47 GMT, Aleksey Shipilev wrote: > `Unsafe.storeStoreFence` currently delegates to stronger `Unsafe.storeFence`. > We can teach compilers to map this directly to already existing rules that > handle `MemBarStoreStore`. Like explicit `LoadFence`/`StoreFence`, we > introduce the special node to differentiate explicit fence and implicit > store-store barriers. `storeStoreFence` is usually used to simulate safe > `final`-field like constructions in special JDK classes, like > `ConstantCallSite` and friends. > > Motivational performance difference on benchmarks from JDK-8276054 on ARM32 > (Raspberry Pi 4): > > > Benchmark Mode Cnt ScoreError Units > Multiple.plain avgt3 2.669 ± 0.004 ns/op > Multiple.release avgt3 16.688 ± 0.057 ns/op > Multiple.storeStoreavgt3 14.021 ± 0.144 ns/op // Better > > MultipleWithLoads.plainavgt3 4.672 ± 0.053 ns/op > MultipleWithLoads.release avgt3 16.689 ± 0.044 ns/op > MultipleWithLoads.storeStore avgt3 14.012 ± 0.010 ns/op // Better > > MultipleWithStores.plain avgt3 14.687 ± 0.009 ns/op > MultipleWithStores.release avgt3 45.393 ± 0.192 ns/op > MultipleWithStores.storeStore avgt3 38.048 ± 0.033 ns/op // Better > > Publishing.plain avgt3 27.079 ± 0.201 ns/op > Publishing.release avgt3 27.088 ± 0.241 ns/op > Publishing.storeStore avgt3 27.009 ± 0.259 ns/op // Within > error, hidden by allocation > > Single.plain avgt3 2.670 ± 0.002 ns/op > Single.releaseFenceavgt3 6.675 ± 0.001 ns/op > Single.storeStoreFence avgt3 8.012 ± 0.027 ns/op // Worse, > seems to be ARM32 implementation artifact > > > The same thing on AArch64 (Raspberry Pi 3): > > > Benchmark Mode Cnt Score Error Units > > Multiple.plain avgt3 5.914 ± 0.115 ns/op > Multiple.release avgt3 10.149 ± 0.059 ns/op > Multiple.storeStoreavgt3 6.757 ± 0.138 ns/op // Better > > MultipleWithLoads.plainavgt3 11.849 ± 0.331 ns/op > MultipleWithLoads.release avgt3 35.565 ± 1.144 ns/op > MultipleWithLoads.storeStore avgt3 19.441 ± 0.471 ns/op // Better > > MultipleWithStores.plain avgt3 5.920 ± 0.213 ns/op > MultipleWithStores.release avgt3 20.286 ± 0.347 ns/op > MultipleWithStores.storeStore avgt3 12.686 ± 0.230 ns/op // Better > > Publishing.plain avgt3 22.261 ± 1.630 ns/op > Publishing.release avgt3 22.269 ± 0.576 ns/op > Publishing.storeStore avgt3 17.464 ± 0.397 ns/op // Better > > Single.plain avgt3 5.916 ± 0.063 ns/op > Single.release avgt3 10.148 ± 0.401 ns/op > Single.storeStore avgt3 6.767 ± 0.164 ns/op // Better > > > As expected, this does not affect x86_64 at all, because both `release` and > `storeStore` are effectively no-ops, only affecting compiler optimizations: > > > Benchmark Mode Cnt Score Error Units > > Multiple.plain avgt3 0.406 ± 0.002 ns/op > Multiple.release avgt3 0.409 ± 0.018 ns/op > Multiple.storeStoreavgt3 0.406 ± 0.001 ns/op > > MultipleWithLoads.plainavgt3 4.328 ± 0.006 ns/op > MultipleWithLoads.release avgt3 4.600 ± 0.014 ns/op > MultipleWithLoads.storeStore avgt3 4.602 ± 0.006 ns/op > > MultipleWithStores.plain avgt3 0.812 ± 0.001 ns/op > MultipleWithStores.release avgt3 0.812 ± 0.002 ns/op > MultipleWithStores.storeStore avgt3 0.812 ± 0.002 ns/op > > Publishing.plain avgt3 6.370 ± 0.059 ns/op > Publishing.release avgt3 6.358 ± 0.436 ns/op > Publishing.storeStore avgt3 6.367 ± 0.054 ns/op > > Single.plain avgt3 0.407 ± 0.039 ns/op > Single.releaseFenceavgt3 0.406 ± 0.001 ns/op > Single.storeStoreFence avgt3 0.406 ± 0.001 ns/op > > > Additional testing: > - [x] Linux x86_64 fastdebug `tier1` > - [x] Linux AArch64 fastdebug `tier1` > - [x] Linux x86_64 Fences benchmark > - [x] Linux AArch64 Fences benchmark > - [x] Linux ARM32 Fences benchmark > - [x] Linux AArch64 jcstress `quick` run This pull request has now been integrated. Changeset: b7a06be9 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/b7a06be98d3057dac4adbb7f4071ac62cf88fe52 Stats: 38 lines in 16 files changed: 32 ins; 5 del; 1 mod 8252990: Intrinsify Unsafe.storeStoreFence Reviewed-by: dholmes, thartmann, whuang - PR: https://git.openjdk.java.net/jdk/pull/6136
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Tue, 2 Nov 2021 00:24:12 GMT, Paul Sandoz wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains 17 commits:
>>
>> - Add cache for memory address var handles
>> - Merge branch 'master' into JEP-419
>> - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
>> - Merge branch 'master' into JEP-419
>> - Fix copyright header in TestArrayCopy
>> - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
>> - * use `invokeWithArguments` to simplify new test
>> - Add test for liveness check with high-aririty downcalls
>>(make sure that if an exception occurs in a downcall because of liveness,
>>ref count of other resources are left intact).
>> - * Fix javadoc issue in VaList
>>* Fix bug in concurrent logic for shared scope acquire
>> - Address review comments
>> - ... and 7 more:
>> https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343
>
> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java line
> 111:
>
>> 109: class VarHandleCache {
>> 110: private static final Map handleMap
>> = new ConcurrentHashMap<>();
>> 111: private static final Map
>> handleMapNoAlignCheck = new ConcurrentHashMap<>();
>
> Something to consider later if this is an issue. Since the number of
> `ValueLayout` instances is fixed, carrier x order = 18, we can use stable
> arrays with ordinals on the instances.
What about alignment?
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Integrated: 8275735: [linux] Remove deprecated Metrics api (kernel memory limit)
On Thu, 28 Oct 2021 13:03:56 GMT, Severin Gehwolf wrote: > Please review this change to remove some API which no longer works as > expected as recent OCI runtimes start to drop support for `--kernel-memory` > switch. See the bug for references. This part of the API is not present in > hotspot code. > > Testing: Container tests (cgroup v1) on Linux x86_64 (all pass) This pull request has now been integrated. Changeset: 9971a2ca Author:Severin Gehwolf URL: https://git.openjdk.java.net/jdk/commit/9971a2cab3892a17f3fd39243df5ecfff5b9f108 Stats: 104 lines in 6 files changed: 0 ins; 100 del; 4 mod 8275735: [linux] Remove deprecated Metrics api (kernel memory limit) Reviewed-by: hseigel, mchung - PR: https://git.openjdk.java.net/jdk/pull/6156
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
> Prompted by a request from Volkan Yazıcı I took a look at why the java.time > formatters are less efficient for some common patterns than custom formatters > in apache-commons and log4j. This patch reduces the gap, without having > looked at the third party implementations. > > When printing times: > - Avoid turning integral values into `String`s before appending them to the > buffer > - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of > `BigDecimal` > > This means a speed-up and reduction in allocations when formatting almost any > date or time pattern, and especially so when including sub-second parts > (`S-S`). > > Much of the remaining overhead can be traced to the need to create a > `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` > internally. We could likely also win performance by specializing some common > patterns. > > Testing: tier1-3 Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add fallback for values outside the allowable range - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/ee13139a..21092323 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=01-02 Stats: 12 lines in 1 file changed: 11 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Tue, 2 Nov 2021 07:31:09 GMT, Stephen Colebourne wrote: >> I see what you're saying that an arbitrary `Temporal` could define its own >> fields with its own ranges, but I would consider it a design bug if such an >> implementation at a whim redefines the value ranges of well-defined >> constants such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect >> such a `Temporal` would have to define its own enumeration of allowed >> `TemporalField`s. > > That isn't the design model however. The design model for the formatter is a > `Map` like view of field to value. Any value may be associated with any field > - that is exactly what `Temporal` offers. > [`TempralAccessor.getLong()`](https://download.java.net/java/early_access/loom/docs/api/java.base/java/time/temporal/TemporalAccessor.html#getLong(java.time.temporal.TemporalField)) > is very explicit about this. > > As indicated above, the positive part is that an hour-of-day of 26 can be > printed by a user-written `WrappingLocalTime` class. The downside is the > inability to make optimizing assumptions as per this code. > > FWIW, I had originally intended to write dedicated private formatters where > the pattern and type to be formatted are known, such as `LocalDate` and the > ISO pattern. Ok, I've added a fallback to instantiate and use an instance of `FractionPrinterParser` when the value is outside of the range. This has a negligible negative effect on performance in the provided micro-benchmarks. Does this resolve your concerns? I think dedicated private formatters is still a good idea, but I wanted to take a stab (like this) at improving common patterns in the shared code first. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v7]
On Tue, 2 Nov 2021 09:54:38 GMT, Ravi Reddy wrote: >> Hi all, >> >> Please review this fix for Infinite loop in ZipOutputStream.close(). >> The main issue here is when ever there is an exception during close >> operations on GZip we are not setting the deflator to a finished state which >> is leading to an infinite loop when we try writing on the same GZip >> instance( since we use while(!def.finished()) inside the write operation). >> >> Thanks, >> Ravi > > Ravi Reddy has updated the pull request incrementally with one additional > commit since the last revision: > > 8193682 : Infinite loop in ZipOutputStream.close() Hello All . Thanks for reviewing these changes. While testing ZipOutputStream:closeEntry(), I have found the same infinite loop issue is reproducible. Since closeEntry() does internally close the deflater , even though the documentation does not specify it, I think it should be fine to give the similar fix in closeEntry() of ZipOutputStream as well. So there are total of three places where we should close deflater before throwing an exception. 1.GZipOutputStream:finish() 2.DeflaterOutputStream:close() 3.ZipOutputStream:closeEntry() I have created a CSR explaining the changes: https://bugs.openjdk.java.net/browse/JDK-8276305 - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
On 02/11/2021 06:38, Jaikiran Pai wrote:
:
Perhaps run 1 writing the hash code of each of the boot modules'
descriptor into a file and then run 2 reading that same file to
compare the hash codes would be one way to do it. But I think that
would just make this test more complex, which I think is avoidable.
I'm not sure that we really need this. There are several jlink tests
that check reproducibility, we think one of them is failing
intermittently with this issue already. So maybe we should leave build
reproducibility to the jlink tests and expand them as needed.
For ModuleDescriptor::hashCode then I was hoping we keep it simple and
just test that the hashCode of the descriptors for modules in the boot
layer matches the hashCode computed from re-parsing the module-info
files, e.g. something simple like this:
for (Module module : ModuleLayer.boot().modules()) {
System.out.println(module);
ModuleDescriptor descriptor1 = module.getDescriptor();
ModuleDescriptor descriptor2;
try (InputStream in =
module.getResourceAsStream("module-info.class")) {
descriptor2 = ModuleDescriptor.read(in);
}
assertEquals(descriptor1, descriptor2);
assertEquals(descriptor1.hashCode(), descriptor2.hashCode());
assertEquals(descriptor1.compareTo(descriptor2), 0);
}
It run can twice, with with the default, the other with -Xshare:off.
One other thought on the change to ModuleDescriptor is that we could
drop the modifiers from the computation of the hashCode of
ModuleDescriptor, Requires, Exports, Opens. Summing the ordinals is okay
but it doesn't give a good spread if you really have modules that have
everything the same except for the modifiers then it doesn't really help.
-Alan
Integrated: 8275766: (tz) Update Timezone Data to 2021e
On Thu, 28 Oct 2021 01:02:27 GMT, Yoshiki Sato wrote: > Please review the integration of tzdata2021e (including tzdata2021d) to the > JDK. > The fix has passed all relevant JTREG regression tests and JCK tests. > > 8275754: (tz) Update Timezone Data to 2021d > 8275849: TestZoneInfo310.java fails with tzdata2021e This pull request has now been integrated. Changeset: 5b4e3986 Author:Yoshiki Sato Committer: Sean Coffey URL: https://git.openjdk.java.net/jdk/commit/5b4e39863dbc0d61e91675261dd6887f704ab868 Stats: 52 lines in 6 files changed: 29 ins; 6 del; 17 mod 8275766: (tz) Update Timezone Data to 2021e 8275849: TestZoneInfo310.java fails with tzdata2021e Reviewed-by: naoto, iris, erikj, coffeys - PR: https://git.openjdk.java.net/jdk/pull/6144
Re: New candidate JEP: 421: Deprecate Finalization for Removal
On a side note, will the actual removal of finalization become a dependency of the actual removal of the security manager? I recall when the security manager was deprecated for removal, developers pointed out that there can be security risks with finalization in the mailing list. On Mon, Nov 1, 2021 at 2:58 PM wrote: > > https://openjdk.java.net/jeps/421 > > Summary: Deprecate finalization for removal in a future > release. Finalization remains enabled by default for now, but can > be disabled to facilitate early testing. In a future release it > will be disabled by default, and in a later release it will be > removed. Maintainers of libraries and applications that rely upon > finalization should consider migrating to other resource management > techniques such as the try-with-resources statement and cleaners. > > - Mark
Re: New candidate JEP: 421: Deprecate Finalization for Removal
On 02/11/2021 14:00, - wrote: On a side note, will the actual removal of finalization become a dependency of the actual removal of the security manager? I recall when the security manager was deprecated for removal, developers pointed out that there can be security risks with finalization in the mailing list. I suspect you may be thinking about classes that specify SM permission checks in their constructors. If so then no SM means the permission check doesn't do anything. If finalization is disabled or removed then the specific attack isn't a concern. So I think independent for that discussion, assuming this is what you mean. -Alan
Re: New candidate JEP: 421: Deprecate Finalization for Removal
Indeed. I was reminded of a discussion [1][2] back in July when I saw this JEP candidate. Now looking at it again, I guess we can easily prevent this risk once this JEP is implemented (so finalizers are no-op and attacks cannot happen even if the security manager is disallowed), and we might not need to remove finalizers before security managers as long as we can disable finalizers. https://mail.openjdk.java.net/pipermail/jdk-dev/2021-July/thread.html#5805 https://mail.openjdk.java.net/pipermail/jdk-dev/2021-July/005808.html On Tue, Nov 2, 2021 at 9:20 AM Alan Bateman wrote: > > On 02/11/2021 14:00, - wrote: > > On a side note, will the actual removal of finalization become a > > dependency of the actual removal of the security manager? I recall > > when the security manager was deprecated for removal, developers > > pointed out that there can be security risks with finalization in the > > mailing list. > > > I suspect you may be thinking about classes that specify SM permission > checks in their constructors. If so then no SM means the permission > check doesn't do anything. If finalization is disabled or removed then > the specific attack isn't a concern. So I think independent for that > discussion, assuming this is what you mean. > > -Alan
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Mon, 1 Nov 2021 22:36:40 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-419 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/419 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Tweak javadoc of loaderLookup Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Tue, 2 Nov 2021 10:30:42 GMT, Maurizio Cimadamore
wrote:
>> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java line
>> 111:
>>
>>> 109: class VarHandleCache {
>>> 110: private static final Map handleMap
>>> = new ConcurrentHashMap<>();
>>> 111: private static final Map
>>> handleMapNoAlignCheck = new ConcurrentHashMap<>();
>>
>> Something to consider later if this is an issue. Since the number of
>> `ValueLayout` instances is fixed, carrier x order = 18, we can use stable
>> arrays with ordinals on the instances.
>
> What about alignment?
Drat, `skipAlignmentCheck` misled me but perhaps there is still benefit for
common constants with 8 bit and size alignment and fallback otherwise.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Integrated: 8276188: Clarify "default charset" descriptions in String class
On Mon, 1 Nov 2021 18:31:17 GMT, Naoto Sato wrote: > This is a leftover document fix to `String` class for the JEP 400. > Corresponding CSR has also been drafted: > https://bugs.openjdk.java.net/browse/JDK-8276238 This pull request has now been integrated. Changeset: 495c828a Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/495c828ae95205885b091dde795b517ba332a2b1 Stats: 16 lines in 1 file changed: 2 ins; 2 del; 12 mod 8276188: Clarify "default charset" descriptions in String class Reviewed-by: iris, joehw - PR: https://git.openjdk.java.net/jdk/pull/6198
RFR: 8276348: Use blessed modifier order in java.base
This PR follows up one of the recent PRs, where I used a non-canonical modifier order. Since the problem was noticed [^1], why not to address it at mass? As far as I remember, the first mass-canonicalization of modifiers took place in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 files. Since then modifiers have become a bit lose, and it makes sense to re-bless (using the JDK-8136583 terminology) them. This change was produced by running the below command followed by updating the copyright years on the affected files where necessary: $ sh ./bin/blessed-modifier-order.sh src/java.base The resulting change is much smaller than that of 2015: 39 lines spanning 21 files. [^1]: https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) [^2]: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/6213/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6213&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276348 Stats: 39 lines in 21 files changed: 0 ins; 0 del; 39 mod Patch: https://git.openjdk.java.net/jdk/pull/6213.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6213/head:pull/6213 PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it at > mass? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit lose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html A colleague suggested that I should clarify that the `blessed-modifier-order.sh` script that I used is available in the JDK repo at https://github.com/openjdk/jdk/blob/01105d6985b39d4064b9066eab3612da5a401685/bin/blessed-modifier-order.sh. That script was contributed by Martin Buchholz in JDK-8136656 in 2015. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling [v3]
On Tue, 2 Nov 2021 06:25:33 GMT, Aleksey Shipilev wrote: >> This blocks JDK-8276215: `StrictMath` intrinsics are handled peculiarly by >> giving failing intrinsics a second chance to match against the similar >> `Math` intrinsics. This has interesting consequence for matchers: we can >> match the native `StrictMath.sqrt` to non-native intrinsic for `Math.sqrt`. >> Interpreter would then have to disambiguate the two. It could be made >> simpler and more consistent. >> >> For `min`/`max` methods, `StrictMath` already delegates to `Math` methods, >> so we can just drop the intrinsics for them. `sqrt` is harder to delegate, >> because it is `native` and a part of public API, so we can instead do the >> proper special intrinsic for it. >> >> There seem to be no performance regressions with this patch at least on >> Linux x86_64: >> >> >> $ CONF=linux-x86_64-server-release make test TEST="micro:StrictMathBench" >> >> Benchmark Mode Cnt Score Error Units >> >> ### Before >> >> StrictMathBench.minDouble thrpt4 230921.558 ± 234.238 ops/ms >> StrictMathBench.minFloat thrpt4 230932.303 ± 126.721 ops/ms >> StrictMathBench.minInt thrpt4 230917.256 ± 73.008 ops/ms >> StrictMathBench.minLongthrpt4 194460.828 ± 178.079 ops/ms >> >> >> StrictMathBench.maxDouble thrpt4 230983.180 ± 161.211 ops/ms >> StrictMathBench.maxFloat thrpt4 230969.290 ± 277.500 ops/ms >> StrictMathBench.maxInt thrpt4 231033.581 ± 200.015 ops/ms >> StrictMathBench.maxLongthrpt4 194590.744 ± 114.295 ops/ms >> >> >> StrictMathBench.sqrtDouble thrpt4 230722.037 ± .080 ops/ms >> >> ### After >> >> StrictMathBench.minDouble thrpt4 230976.625 ± 67.338 ops/ms >> StrictMathBench.minFloat thrpt4 230896.021 ± 270.434 ops/ms >> StrictMathBench.minInt thrpt4 230859.741 ± 403.147 ops/ms >> StrictMathBench.minLongthrpt4 194456.673 ± 111.557 ops/ms >> >> StrictMathBench.maxDouble thrpt4 230890.776 ± 89.924 ops/ms >> StrictMathBench.maxFloat thrpt4 230918.334 ± 63.160 ops/ms >> StrictMathBench.maxInt thrpt4 231059.128 ± 51.224 ops/ms >> StrictMathBench.maxLongthrpt4 194488.210 ± 495.224 ops/ms >> >> StrictMathBench.sqrtDouble thrpt4 231023.703 ± 247.330 ops/ms >> >> >> Additional testing: >> - [x] `StrictMath` benchmarks >> - [x] Linux x86_64 fastdebug `tier1` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Keep intrinsics on StrictMath Good. Thank you for fixing it. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6184
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it at > mass? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit lose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html Marked as reviewed by dfuchs (Reviewer). LGTM - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote:
> This PR follows up one of the recent PRs, where I used a non-canonical
> modifier order. Since the problem was noticed [^1], why not to address it at
> mass?
>
> As far as I remember, the first mass-canonicalization of modifiers took place
> in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453
> files. Since then modifiers have become a bit lose, and it makes sense to
> re-bless (using the JDK-8136583 terminology) them.
>
> This change was produced by running the below command followed by updating
> the copyright years on the affected files where necessary:
>
> $ sh ./bin/blessed-modifier-order.sh src/java.base
>
> The resulting change is much smaller than that of 2015: 39 lines spanning 21
> files.
>
> [^1]:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
> (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
> [^2]:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html
src/java.base/share/classes/java/lang/Object.java line 282:
> 280: * For objects of type {@code Class,} by executing a
> 281: * static synchronized method of that class.
> 282: *
In comments, I think the 'synchronized static 'reads better, the conventional
order is for the code.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it at > mass? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit lose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it at > mass? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit lose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Mon, 1 Nov 2021 22:36:40 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak javadoc of loaderLookup
Mostly some minor javadoc comments.
src/java.base/share/classes/java/lang/Module.java line 32:
> 30: import java.lang.annotation.Annotation;
> 31: import java.lang.invoke.MethodHandle;
> 32: import java.lang.invoke.VarHandle;
These imports seem spurious now.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
line 177:
> 175: }
> 176: if (carrier.isPrimitive() &&
> Wrapper.forPrimitiveType(carrier).bitWidth() != size &&
> 177: carrier != boolean.class && size != 8) {
I find this condition hard to parse, I'd suggest re-writing it as:
if (carrier.isPrimitive()) {
long expectedSize = carrier == boolean.class ? 8 :
Wrapper.forPrimitiveType(carrier).bitWidth();
if (size != expectedSize) {
throw ...
}
}
(Maybe even change the `if` to an `else` and combine it with the above if).
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
line 484:
> 482: public static final class OfAddress extends ValueLayout {
> 483: OfAddress(ByteOrder order) {
> 484: super(MemoryAddress.class, order, Unsafe.ADDRESS_SIZE * 8);
I see `Unsafe.ADDRESS_SIZE` used in several places, suggest to maybe add an
`ADDRESS_SIZE_BITS` constants somewhere (it's a bit more readable).
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
line 42:
> 40: final long blockSize;
> 41: final long arenaSize;
> 42: final ResourceScope scope;
Could these field be made private?
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
line 88:
> 86: if (size > arenaSize) {
> 87: throw new OutOfMemoryError();
> 88: }
Isn't this already covered by the `finally` block? Also, this seems to be
checking the unaltered `size`, which I think should have been already done at
the end of the previous `allocate` call right?
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java
line 122:
> 120: ResourceScopeImpl targetImpl = (ResourceScopeImpl)target;
> 121: targetImpl.acquire0();
> 122: addCloseAction(targetImpl::release0);
Maybe this should explicitly check if target is `null` (though the call to
`acquire0` would also produce an NPE, the stack trace having
Objects::requireNonNull in there would make the error more obvious I think).
Suggestion:
public void keepAlive(ResourceScope target) {
Objects.requireNonNull(target);
if (target == this) {
throw new IllegalArgumentException("Invalid target scope.");
}
ResourceScopeImpl targetImpl = (ResourceScopeImpl)target;
targetImpl.acquire0();
addCloseAction(targetImpl::release0);
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java
line 101:
> 99: int value;
> 100: do {
> 101: value = (int)
> STATE.getVolatile(jdk.internal.foreign.SharedScope.this);
Doesn't need to be fully qualified I think?
Suggestion:
value = (int) STATE.getVolatile(this);
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java
line 106:
> 104: throw new IllegalStateException("Already closed");
> 105: }
> 106: } while
> (!STATE.compareAndSet(jdk.internal.foreign.SharedScope.this, value, value -
> 1));
Same here
Suggestion:
} while (!STATE.compareAndSet(this, value, value - 1));
-
Marked as reviewed by jvernee (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Mon, 1 Nov 2021 12:05:32 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains 17 commits:
>
> - Add cache for memory address var handles
> - Merge branch 'master' into JEP-419
> - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
> - Merge branch 'master' into JEP-419
> - Fix copyright header in TestArrayCopy
> - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
> - * use `invokeWithArguments` to simplify new test
> - Add test for liveness check with high-aririty downcalls
>(make sure that if an exception occurs in a downcall because of liveness,
>ref count of other resources are left intact).
> - * Fix javadoc issue in VaList
>* Fix bug in concurrent logic for shared scope acquire
> - Address review comments
> - ... and 7 more:
> https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343
src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1586:
> 1584: public void ensureCustomized(MethodHandle mh) {
> 1585: mh.customize();
> 1586: }
This is no longer needed, but it probably got picked up in the merge.
src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java line
144:
> 142: * @param mh the method handle
> 143: */
> 144: void ensureCustomized(MethodHandle mh);
Same here, no longer needed. (it was used by now removed upcall handler code.
See https://github.com/openjdk/panama-foreign/pull/553)
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java
line 107:
> 105: *
> 106: * @param offset offset in bytes (relative to this address). The
> final address of this read operation can be expressed as {@code
> toRowLongValue() + offset}.
> 107: * @return a Java UTF-8 string containing all the bytes read from
> the given starting address ({@code toRowLongValue() + offset})
(see also comment on MemorySegment.getUtf8String)
Suggestion:
* @return a Java string constructed from the bytes read from the given
starting address ({@code toRowLongValue() + offset})
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 387:
> 385:
> 386: /**
> 387: * Performs an element-wise bulk copy from given source segment to
> this segment. More specifically, the bytes at
Suggestion:
* Performs a byte-wise bulk copy from given source segment to this
segment. More specifically, the bytes at
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 400:
> 398: * a multiple of the source element layout size, if the source
> segment is incompatible with the alignment constraints
> 399: * in the source element layout, or if this segment is incompatible
> with the alignment constraints
> 400: * in the destination element layout.
This speaks about element layouts, but I don't see any element layouts in the
method implementation.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 633:
> 631: * java.nio.charset.CharsetDecoder} class should be used when more
> control
> 632: * over the decoding process is required.
> 633: * @param offset offset in bytes (relative to this segment). For
> instance, if this segment is a {@link #isNative()} segment,
Suggestion:
* @param offset offset in bytes (relative to this segment). For instance,
if this segment is a {@link #isNative() native} segment,
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 636:
> 634: * the final address of this read operation can be
> expressed as {@code address().toRowLongValue() + offset}.
> 635: * @return a Java UTF-8 string containing all the bytes read from
> the given starting address up to (but not including)
> 636: * the first {@code '\0'} terminator character (assuming one is
> found).
The phrase "a Java UTF-8 string" sounds strange to me, as Java Strings are not
encoded in UTF-8. The string that is read is UTF-8 encoded, but then it is
converted from UTF-8 to Java internal String encoding (UTF-16 or Latin1).
I'd suggest just dropping the 'UTF-8', and changing 'containing all' to
'constructed from'.
Suggestion:
* @return a Java string constructed from the bytes read from the given
starting address up to (but not including)
* the first {@code '\0'} terminator character (assuming one is found).
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 652:
> 650: * java.nio.charset.CharsetDecoder} class should
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Mon, 1 Nov 2021 15:38:18 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains 17 commits:
>>
>> - Add cache for memory address var handles
>> - Merge branch 'master' into JEP-419
>> - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
>> - Merge branch 'master' into JEP-419
>> - Fix copyright header in TestArrayCopy
>> - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
>> - * use `invokeWithArguments` to simplify new test
>> - Add test for liveness check with high-aririty downcalls
>>(make sure that if an exception occurs in a downcall because of liveness,
>>ref count of other resources are left intact).
>> - * Fix javadoc issue in VaList
>>* Fix bug in concurrent logic for shared scope acquire
>> - Address review comments
>> - ... and 7 more:
>> https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
> line 1035:
>
>> 1033: *
>> 1034: * @param layout the layout of the memory region to be read.
>> 1035: * @param offset offset in bytes (relative to this segment). For
>> instance, if this segment is a {@link #isNative()} segment,
>
> Suggestion:
>
> * @param offset offset in bytes (relative to this segment). For
> instance, if this segment is a {@link #isNative() native} segment,
Same suggestion with all the other getters/setters below (I assume you wanted
to add text to the link here?)
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
> line 1549:
>
>> 1547: * @param index index (relative to this segment). For instance, if
>> this segment is a {@link #isNative()} segment,
>> 1548: * the final address of this write operation can be
>> expressed as {@code address().toRowLongValue() + (index *
>> layout.byteSize())}.
>> 1549: * @param value the byte value to be written.
>
> Suggestion:
>
> * @param value the address value to be written.
I think all the setters have this problem.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
RFR: 8276141: XPathFactory set/getProperty method
Add setProperty/getProperty methods to the XPath API so that properties can be supported in the future. - Commit messages: - 8276141: XPathFactory set/getProperty method Changes: https://git.openjdk.java.net/jdk/pull/6215/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6215&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276141 Stats: 51 lines in 1 file changed: 51 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6215.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6215/head:pull/6215 PR: https://git.openjdk.java.net/jdk/pull/6215
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 17:13:47 GMT, Roger Riggs wrote: > In comments, I think the 'synchronized static 'reads better, the conventional > order is for the code. I think Roger is right and maybe the change to the javadoc could be dropped from this patch. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 17:39:17 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/lang/Object.java line 282:
>>
>>> 280: * For objects of type {@code Class,} by executing a
>>> 281: * static synchronized method of that class.
>>> 282: *
>>
>> In comments, I think the 'synchronized static 'reads better, the
>> conventional order is for the code.
>
>> In comments, I think the 'synchronized static 'reads better, the
>> conventional order is for the code.
>
> I think Roger is right and maybe the change to the javadoc could be dropped
> from this patch.
It's tough when a natural language clashes with a programming language. I
appreciate that this particular clash might cause discomfort to native English
speakers. (This reminds me of that _DOSASCOMP_ mnemonic for adjective order.)
That said, consider the following pragmatic aspect. Unless we change the script
not to process prose in comments (btw, how would we do that?), every single
time we run that script, that particular line in Object.java will need to be
tracked and excluded.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276141: XPathFactory set/getProperty method
On Tue, 2 Nov 2021 17:31:40 GMT, Joe Wang wrote: > Add setProperty/getProperty methods to the XPath API so that properties can > be supported in the future. Are you planning to add tests? Also don't forget @since. - PR: https://git.openjdk.java.net/jdk/pull/6215
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 17:45:07 GMT, Pavel Rappo wrote: >>> In comments, I think the 'synchronized static 'reads better, the >>> conventional order is for the code. >> >> I think Roger is right and maybe the change to the javadoc could be dropped >> from this patch. > > It's tough when a natural language clashes with a programming language. I > appreciate that this particular clash might cause discomfort to native > English speakers. (This reminds me of that _DOSASCOMP_ mnemonic for adjective > order.) That said, consider the following pragmatic aspect. Unless we change > the script not to process prose in comments (btw, how would we do that?), > every single time we run that script, that particular line in Object.java > will need to be tracked and excluded. Here's a bit of archaeology. I found the original JDK-8136583 patch, which has moved from where it was in the RFR to https://cr.openjdk.java.net/~martin/webrevs/jdk9/blessed-modifier-order/. That patch doesn't change Object.java. The RFR thread mentions neither that javadoc change nor any javadoc change for that matter. So either the script was different, or Martin filtered that line (from Object.java) out before creating the webrev. Now, in his followup thread on core-libs-dev, http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035273.html, Martin specifically pointed out javadoc changes and said that they seem fine to him. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it at > mass? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit lose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html JFYI a couple of times I've wondered if we regressed on this. I just ran the script on the desktop module and we havea few instances there too, so I've filed a clean up bug on it. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 18:48:06 GMT, Mark Sheppard wrote: >> Here's a bit of archaeology. I found the original JDK-8136583 patch, which >> has moved from where it was in the RFR to >> https://cr.openjdk.java.net/~martin/webrevs/jdk9/blessed-modifier-order/. >> That patch doesn't change Object.java. The RFR thread mentions neither that >> javadoc change nor any javadoc change for that matter. So either the script >> was different, or Martin filtered that line (from Object.java) out before >> creating the webrev. >> >> Now, in his followup thread on core-libs-dev, >> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035273.html, >> Martin specifically pointed out javadoc changes and said that they seem >> fine to him. > > "to each his own". I think static synchronized reads best and more natural > than synchronzied static. Also from a semantic point of view as it conveys > class level characteristic thus lends static to having a prominence in > specified order. Pragmatically, fix the script to ignore those keywords on comment lines. Learn Perl, its just a regular expression pattern match and replace expression. All of the changes have to be manually reviewed by the author and then the reviewers. Checking unneeded changes is part of every mechanical change. The text being changed in the javadoc is the *spec*; that deserves special attention in review. But having seen several reviewers be unmoved by the difference, the real pragmatic view is to ignore the English. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 18:17:36 GMT, Pavel Rappo wrote: >> It's tough when a natural language clashes with a programming language. I >> appreciate that this particular clash might cause discomfort to native >> English speakers. (This reminds me of that _DOSASCOMP_ mnemonic for >> adjective order.) That said, consider the following pragmatic aspect. Unless >> we change the script not to process prose in comments (btw, how would we do >> that?), every single time we run that script, that particular line in >> Object.java will need to be tracked and excluded. > > Here's a bit of archaeology. I found the original JDK-8136583 patch, which > has moved from where it was in the RFR to > https://cr.openjdk.java.net/~martin/webrevs/jdk9/blessed-modifier-order/. > That patch doesn't change Object.java. The RFR thread mentions neither that > javadoc change nor any javadoc change for that matter. So either the script > was different, or Martin filtered that line (from Object.java) out before > creating the webrev. > > Now, in his followup thread on core-libs-dev, > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035273.html, > Martin specifically pointed out javadoc changes and said that they seem fine > to him. "to each his own". I think static synchronized reads best and more natural than synchronzied static. Also from a semantic point of view as it conveys class level characteristic thus lends static to having a prominence in specified order. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: 8276207: Properties.loadFromXML/storeToXML works incorrectly for supplementary characters
Hi Joe,
Thanks. I have created a pull request[1].
The new entry in "testLoadAndStore" will do the roundtrip as you have
described.
So "testStoreWithSupplementaryCharacters" is redundant.
I have removed this method before creating the pull request.
[1] : https://github.com/openjdk/jdk/pull/6216
On Tue, 2 Nov 2021 at 14:09, Joe Wang wrote:
> Hi Anirvan,
>
> The change looks good to me. Please go ahead with creating a pull
> request. It would be easier for reviewers to pull the patch and look at
> it. On first glance, test "testStoreWithSupplementaryCharacters" may be
> a bit sensitive to file format. It may be solved with a roundtrip
> (store/read) and comparing key/value of the entry, or test that the
> output contains then entry element.
>
> Thanks,
> Joe
>
> On 10/31/21 3:56 AM, Anirvan Sarkar wrote:
> > Hi,
> >
> > Since it seems that the mailing list is scrubbing attachments, patch is
> > mentioned inline below.
> >
> > diff a/src/java.base/share/classes/jdk/internal/util/xml/impl/Parser.java
> > b/src/java.base/share/classes/jdk/internal/util/xml/impl/Parser.java
> > --- a/src/java.base/share/classes/jdk/internal/util/xml/impl/Parser.java
> > +++ b/src/java.base/share/classes/jdk/internal/util/xml/impl/Parser.java
> > @@ -1,7 +1,7 @@
> > /*
> > - * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights
> > reserved.
> > + * Copyright (c) 2012, 2021, Oracle and/or its affiliates. All rights
> > reserved.
> >* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> >*
> >* This code is free software; you can redistribute it and/or modify it
> >* under the terms of the GNU General Public License version 2 only, as
> >* published by the Free Software Foundation. Oracle designates this
> > @@ -1991,24 +1991,22 @@
> > case ';':
> > // Convert the character entity
> to a
> > character
> > try {
> > int i = Integer.parseInt(
> > new String(mBuff, idx + 1,
> > mBuffIdx - idx), 10);
> > -if (i >= 0x) {
> > -panic(FAULT);
> > +// Restore the buffer offset
> > +mBuffIdx = idx - 1;
> > +for(char character :
> Character.toChars(i))
> > {
> > +if (character == ' ' || mInp.next !=
> > null) {
> > +bappend(character, flag);
> > +} else {
> > +bappend(character);
> > +}
> > }
> > -ch = (char) i;
> > } catch (NumberFormatException nfe) {
> > panic(FAULT);
> > }
> > -// Restore the buffer offset
> > -mBuffIdx = idx - 1;
> > -if (ch == ' ' || mInp.next != null) {
> > -bappend(ch, flag);
> > -} else {
> > -bappend(ch);
> > -}
> > st = -1;
> > break;
> >
> > case 'a':
> > // If the entity buffer is empty
> and
> > ch == 'x'
> > @@ -2032,24 +2030,22 @@
> > case ';':
> > // Convert the character entity
> to a
> > character
> > try {
> > int i = Integer.parseInt(
> > new String(mBuff, idx + 1,
> > mBuffIdx - idx), 16);
> > -if (i >= 0x) {
> > -panic(FAULT);
> > +// Restore the buffer offset
> > +mBuffIdx = idx - 1;
> > +for(char character :
> Character.toChars(i))
> > {
> > +if (character == ' ' || mInp.next !=
> > null) {
> > +bappend(character, flag);
> > +} else {
> > +bappend(character);
> > +}
> > }
> > -ch = (char) i;
> > } catch (NumberFormatException nfe) {
> > panic(FAULT);
> > }
> > -// Restore the buffer offset
> > -
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 18:48:20 GMT, Roger Riggs wrote:
> Pragmatically, fix the script to ignore those keywords on comment lines.
> Learn Perl, its just a regular expression pattern match and replace
> expression.
I understand in principle how to modify that script to ignore doc comments. The
thing I was referring to when said "btw, how would we do that?" was this: not
all comment lines are prose. Some of those lines belong to snippets of code,
which I guess you would also like to be properly formatted.
> But having seen several reviewers be unmoved by the difference, the real
> pragmatic view is to ignore the English.
I'm sorry you feel that way. Would it be okay if I made it clear that those two
words are not English adjectives but are special symbols that happen to use
Latin script and originate from the English words they resemble? If so, I could
enclose each of them in `{@code ... }`. If not, I could drop that particular
change from this PR.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote:
> This PR follows up one of the recent PRs, where I used a non-canonical
> modifier order. Since the problem was noticed [^1], why not to address it at
> mass?
>
> As far as I remember, the first mass-canonicalization of modifiers took place
> in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453
> files. Since then modifiers have become a bit loose, and it makes sense to
> re-bless (using the JDK-8136583 terminology) them.
>
> This change was produced by running the below command followed by updating
> the copyright years on the affected files where necessary:
>
> $ sh ./bin/blessed-modifier-order.sh src/java.base
>
> The resulting change is much smaller than that of 2015: 39 lines spanning 21
> files.
>
> [^1]:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
> (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
> [^2]:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html
src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
> 86: */
> 87: public
> 88: abstract class CallSite {
I think it's better to move all modifiers to the single line.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
RFR: 8276207: Properties.loadFromXML/storeToXML works incorrectly for supplementary characters
This fixes Properties.loadFromXML/storeToXML so that it works correctly for supplementary characters. - Commit messages: - 8276207: Properties.loadFromXML/storeToXML works incorrectly for supplementary characters Changes: https://git.openjdk.java.net/jdk/pull/6216/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6216&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276207 Stats: 90 lines in 3 files changed: 58 ins; 16 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/6216.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6216/head:pull/6216 PR: https://git.openjdk.java.net/jdk/pull/6216
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 19:15:26 GMT, Andrey Turbanov wrote:
>> This PR follows up one of the recent PRs, where I used a non-canonical
>> modifier order. Since the problem was noticed [^1], why not to address it at
>> mass?
>>
>> As far as I remember, the first mass-canonicalization of modifiers took
>> place in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning
>> 453 files. Since then modifiers have become a bit loose, and it makes sense
>> to re-bless (using the JDK-8136583 terminology) them.
>>
>> This change was produced by running the below command followed by updating
>> the copyright years on the affected files where necessary:
>>
>> $ sh ./bin/blessed-modifier-order.sh src/java.base
>>
>> The resulting change is much smaller than that of 2015: 39 lines spanning 21
>> files.
>>
>> [^1]:
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
>> (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
>> [^2]:
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html
>
> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>
>> 86: */
>> 87: public
>> 88: abstract class CallSite {
>
> I think it's better to move all modifiers to the single line.
This is a survivorship bias. This example jumps out at you, because it happens
to use missorted modifiers. I'm pretty sure this is not an aberration, but a
style. If you want to change that style, you should create a respective JBS
issue.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Tue, 2 Nov 2021 16:51:06 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Tweak javadoc of loaderLookup
>
> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
> line 88:
>
>> 86: if (size > arenaSize) {
>> 87: throw new OutOfMemoryError();
>> 88: }
>
> Isn't this already covered by the `finally` block? Also, this seems to be
> checking the unaltered `size`, which I think should have been already done at
> the end of the previous `allocate` call right?
I'll have to think some more about this. I don't think this is covered inside
the block - that is, the block tries to allocate, and then in the finally we
throw if we realized we've allocated too much.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v13]
> This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Address impl review comments - Address API review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/7cf4fcd9..1126133a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5907&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5907&range=11-12 Stats: 103 lines in 11 files changed: 8 ins; 23 del; 72 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v13]
On Tue, 2 Nov 2021 19:35:29 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Address impl review comments
> - Address API review comments
src/java.base/share/classes/java/lang/Module.java line 114:
> 112:
> 113: // true, if this module allows restricted native access; @Stable
> makes sure that modules that allow native
> 114: // access capture this property as a constant.
Do you mind fixing this comment to avoid the really long line, it sticks out
compare to everything else around it.
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 478:
> 476: private static final JavaNioAccess NIO_ACCESS =
> SharedSecrets.getJavaNioAccess();
> 477:
> 478: static Runnable acquireScope(ByteBuffer bb, boolean async) {
At some point (not this PR) we should move the "async" out of this file, IOUtil
was for synchronous I/O.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Tue, 2 Nov 2021 18:55:47 GMT, Maurizio Cimadamore wrote: >> I'll have to think some more about this. I don't think this is covered >> inside the block - that is, the block tries to allocate, and then in the >> finally we throw if we realized we've allocated too much. > > What is missing, I think, is a check (size > arenaSize) at the beginning of > the method (we only check this in one of the paths). But we need to check > before and after, I think, as it is possible to allocate a segment and then > realize that we ended up overflowing the arena size. While what I said above correctly reflects what the implementation does, I think a broader issue is that the arena allocator implementation is allocating sometimes more native memory than what its contract specifies. While in some cases we can prevent that, I think in the general case (e.g. where we allocate a new block) we cannot, unless we add extra API guarantees - e.g. that the arena size should be a multiple of the block size (but then we'd have to special case `Long.MAX_VALUE`, or maybe pick a "big enough" power of two instead) - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Tue, 2 Nov 2021 18:48:57 GMT, Maurizio Cimadamore
wrote:
>> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
>> line 88:
>>
>>> 86: if (size > arenaSize) {
>>> 87: throw new OutOfMemoryError();
>>> 88: }
>>
>> Isn't this already covered by the `finally` block? Also, this seems to be
>> checking the unaltered `size`, which I think should have been already done
>> at the end of the previous `allocate` call right?
>
> I'll have to think some more about this. I don't think this is covered inside
> the block - that is, the block tries to allocate, and then in the finally we
> throw if we realized we've allocated too much.
What is missing, I think, is a check (size > arenaSize) at the beginning of the
method (we only check this in one of the paths). But we need to check before
and after, I think, as it is possible to allocate a segment and then realize
that we ended up overflowing the arena size.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Tue, 2 Nov 2021 19:02:51 GMT, Maurizio Cimadamore wrote: >> What is missing, I think, is a check (size > arenaSize) at the beginning of >> the method (we only check this in one of the paths). But we need to check >> before and after, I think, as it is possible to allocate a segment and then >> realize that we ended up overflowing the arena size. > > While what I said above correctly reflects what the implementation does, I > think a broader issue is that the arena allocator implementation is > allocating sometimes more native memory than what its contract specifies. > While in some cases we can prevent that, I think in the general case (e.g. > where we allocate a new block) we cannot, unless we add extra API guarantees > - e.g. that the arena size should be a multiple of the block size (but then > we'd have to special case `Long.MAX_VALUE`, or maybe pick a "big enough" > power of two instead) Maybe we should not support block size in the case of a bounded arena. i.e. just allocate the whole thing upfront, and have 3 APIs: 1. arena with no bounds and default block size. 2. arena with no bounds and custom block size. 3. arena with bounds, that has no blocks size but allocates the whole thing in one go (could be modeled as block size = arena size). Right now we have 1. and 2., but instead of 3. we have a variant that allows setting both the arena size and block size. If we want to keep what we currently have, I'd suggest changing the arena size to a block count for the variant that takes both the arena size and the block size (I think in that case `Long.MAX_VALUE` should still work?). Any ways, that seems like something that could be addressed in 19 as well. - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it at > mass? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit loose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html Keep it as is with the modifiers in the recommended order. I don't think adding extra typography is warranted. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 19:22:15 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>>
>>> 86: */
>>> 87: public
>>> 88: abstract class CallSite {
>>
>> I think it's better to move all modifiers to the single line.
>
> This is a survivorship bias. This example jumps out at you, because it
> happens to use missorted modifiers. I'm pretty sure this is not an
> aberration, but a style. If you want to change that style, you should create
> a respective JBS issue.
I've grepped the code and can now see that a list of modifiers broken by
newlines which cannot be explained by line-width concerns is indeed an
irregularity. Not only in java.base but also in other modules. Although there
aren't many of such cases, I would prefer them to be addressed in a separate
cleanup, which you are welcome to pursue.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently
On Fri, 24 Sep 2021 11:28:09 GMT, Masanori Yano wrote: > Could you please review the 8250678 bug fixes? > > The `parse` method of ModuleDescriptor.Version class behaves incorrectly when > the input string contains consecutive delimiters. > > The `parse` method treats strings as three sections, but the parsing method > differs between the method for the version sections and the ones for others. > In version sections, the `parse` method takes a single character in a loop > and determines whether it is a delimiter. In pre and build sections, the > parse method takes two characters in a loop and determines whether the second > character is the delimiter. Therefore, if the string contains a sequence of > delimiters in pre or build section, the `parse` method treats character at > the odd-numbered position as a token and the one at even-numbered as a > delimiter. > > A string containing consecutive delimiters is an incorrect version string, > but this behavior does not follow the API specification. > https://download.java.net/java/early_access/jdk18/docs/api/java.base/java/lang/module/ModuleDescriptor.Version.html > > Therefore, I propose to fix the parsing method of pre and build section in > the same way as the version. I reviewed the change. It is reasonable to fix the parsing of the pre-release version and the build version be consistent with parsing of the version number which currently skips consecutive delimiters. The spec is unclear on whether an empty token separated by the delimiters is ignored. The spec may needs some clarification. - PR: https://git.openjdk.java.net/jdk/pull/5679
RFR: 8275821: Optimize random number generators developed in JDK-8248862 using Math.unsignedMultiplyHigh()
This change optimizes random number generators using Math.unsignedMultiplyHigh() - Commit messages: - Update Math.java - 8275821: Optimize random number generators developed in JDK-8248862 using Math.unsignedMultiplyHigh() Changes: https://git.openjdk.java.net/jdk/pull/6206/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6206&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275821 Stats: 31 lines in 3 files changed: 0 ins; 28 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6206.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6206/head:pull/6206 PR: https://git.openjdk.java.net/jdk/pull/6206
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it at > mass? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit loose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html Marked as reviewed by martin (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 19:14:23 GMT, Pavel Rappo wrote:
>> Pragmatically, fix the script to ignore those keywords on comment lines.
>> Learn Perl, its just a regular expression pattern match and replace
>> expression.
>>
>> All of the changes have to be manually reviewed by the author and then the
>> reviewers.
>> Checking unneeded changes is part of every mechanical change.
>>
>> The text being changed in the javadoc is the *spec*; that deserves special
>> attention in review.
>>
>> But having seen several reviewers be unmoved by the difference, the real
>> pragmatic view
>> is to ignore the English.
>
>> Pragmatically, fix the script to ignore those keywords on comment lines.
>> Learn Perl, its just a regular expression pattern match and replace
>> expression.
>
> I understand in principle how to modify that script to ignore doc comments.
> The thing I was referring to when said "btw, how would we do that?" was this:
> not all comment lines are prose. Some of those lines belong to snippets of
> code, which I guess you would also like to be properly formatted.
>
>> But having seen several reviewers be unmoved by the difference, the real
>> pragmatic view is to ignore the English.
>
> I'm sorry you feel that way. Would it be okay if I made it clear that those
> two words are not English adjectives but are special symbols that happen to
> use Latin script and originate from the English words they resemble? If so, I
> could enclose each of them in `{@code ... }`. If not, I could drop that
> particular change from this PR.
The blessed-modifier-order.sh script intentionally modifies comments, with the
hope of finding code snippets (it did!)
Probably I manually deleted the change to Object.java back in 2015, to avoid
the sort of controversy we're seeing now.
I don't have a strong feeling either way on changing that file.
I agree with @pavelrappo that script-generated changes should not be mixed
with manual changes.
I would also not update copyright years for such changes.
It's a feature of blessed-modifier-order.sh that all existing formatting is
perfectly preserved.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v14]
> This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix long comment line in Module.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/1126133a..c219ae12 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5907&range=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5907&range=12-13 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8276141: XPathFactory set/getProperty method [v2]
> 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: Add default impl; Add tests. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6215/files - new: https://git.openjdk.java.net/jdk/pull/6215/files/b0b9ae26..569c6cd8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6215&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6215&range=00-01 Stats: 205 lines in 5 files changed: 197 ins; 2 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/6215.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6215/head:pull/6215 PR: https://git.openjdk.java.net/jdk/pull/6215
Re: RFR: 8276141: XPathFactory set/getProperty method [v2]
On Wed, 3 Nov 2021 01:10:46 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: > > Add default impl; Add tests. Thanks Alan. Added "since"; Added default impl; Added tests. Note that I removed the run with security manager from this test. As the Security Manager is deprecated, the run with security manager in tests unrelated to secure processing will need to be removed. - PR: https://git.openjdk.java.net/jdk/pull/6215
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v11]
> Can I please get a review for this change which fixes the issue reported in > https://bugs.openjdk.java.net/browse/JDK-8275509? > > The `ModuleDescriptor.hashCode()` method uses the hash code of its various > components to compute the final hash code. While doing so it ends up calling > hashCode() on (collection of) various `enum` types. Since the hashCode() of > enum types is generated from a call to `java.lang.Object.hashCode()`, their > value isn't guaranteed to stay the same across multiple JVM runs. > > The commit here proposes to use the ordinal of the enum as the hash code. As > Alan notes in the mailing list discussion, any changes to the ordinal of the > enum (either due to addition of new value, removal of a value or just > reordering existing values, all of which I think will be rare in these > specific enum types) isn't expected to produce the same hash code across > those changed runtimes and that should be okay. > > The rest of the ModuleDescriptor.hashCode() has been reviewed too and apart > from calls to the enum types hashCode(), rest of the calls in that > implementation, either directly or indirectly end up as calls on > `java.lang.String.hashCode()` and as such aren't expected to cause > non-deterministic values. > > A new jtreg test has been added to reproduce this issue and verify the fix. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: simplify the test based on review inputs - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/3552edf0..9dd2774c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6078&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6078&range=09-10 Stats: 91 lines in 1 file changed: 5 ins; 67 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8271515: Integration of JEP 417: Vector API (Third Incubator) [v7]
On Wed, 27 Oct 2021 21:42:29 GMT, Paul Sandoz wrote: >> This PR improves the performance of vector operations that accept masks on >> architectures that support masking in hardware, specifically Intel AVX512 >> and ARM SVE. >> >> On architectures that do not support masking in hardware the same technique >> as before is applied to most operations, specifically composition using >> blend. >> >> Masked loads/stores are a special form of masked operation that require >> additional care to ensure out-of-bounds access throw exceptions. The range >> checking has not been fully optimized and will require further work. >> >> No API enhancements were required and only a few additional tests were >> needed. > > Paul Sandoz has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 12 commits: > > - Merge branch 'master' into JDK-8271515-vector-api > - Merge pull request #1 from nsjian/JDK-8271515 > >Address AArch64 review comments from Nick. > - Address review comments from Nick. > - Merge branch 'master' into JDK-8271515-vector-api > - Resolve review comments. > - Merge branch 'master' into JDK-8271515-vector-api > - Apply patch from https://github.com/openjdk/panama-vector/pull/152 > - Apply patch from https://github.com/openjdk/panama-vector/pull/142 > - Apply patch from https://github.com/openjdk/panama-vector/pull/139 > - Apply patch from https://github.com/openjdk/panama-vector/pull/151 > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/9a3e9542...c9a77225 src/hotspot/cpu/aarch64/aarch64_sve_ad.m4 line 2349: > 2347: BasicType to_bt = Matcher::vector_element_basic_type(this); > 2348: Assembler::SIMD_RegVariant to_size = __ > elemType_to_regVariant(to_bt); > 2349: __ sve_fcvtzs(as_FloatRegister($dst$$reg), __ D, ptrue, > as_FloatRegister($src$$reg), __ D); Converting from double to long and then narrow to target types did not follow JLS. I will fix it. Thanks to @fg1417 for helping to find out this issue. - PR: https://git.openjdk.java.net/jdk/pull/5873
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
Hello Alan,
On 02/11/21 5:30 pm, Alan Bateman wrote:
On 02/11/2021 06:38, Jaikiran Pai wrote:
:
Perhaps run 1 writing the hash code of each of the boot modules'
descriptor into a file and then run 2 reading that same file to
compare the hash codes would be one way to do it. But I think that
would just make this test more complex, which I think is avoidable.
I'm not sure that we really need this. There are several jlink tests
that check reproducibility, we think one of them is failing
intermittently with this issue already. So maybe we should leave build
reproducibility to the jlink tests and expand them as needed.
For ModuleDescriptor::hashCode then I was hoping we keep it simple and
just test that the hashCode of the descriptors for modules in the boot
layer matches the hashCode computed from re-parsing the module-info
files, e.g. something simple like this:
for (Module module : ModuleLayer.boot().modules()) {
System.out.println(module);
ModuleDescriptor descriptor1 = module.getDescriptor();
ModuleDescriptor descriptor2;
try (InputStream in =
module.getResourceAsStream("module-info.class")) {
descriptor2 = ModuleDescriptor.read(in);
}
assertEquals(descriptor1, descriptor2);
assertEquals(descriptor1.hashCode(), descriptor2.hashCode());
assertEquals(descriptor1.compareTo(descriptor2), 0);
}
It run can twice, with with the default, the other with -Xshare:off.
Sounds reasonable. I've updated the PR to simplify the test and run it
once with default CDS and once with -Xshare:off. Given this
simplification, it now allows me to use testng for these comparisons, so
I've moved it from regular "main" to testng test case. I hope that's OK.
One other thought on the change to ModuleDescriptor is that we could
drop the modifiers from the computation of the hashCode of
ModuleDescriptor, Requires, Exports, Opens. Summing the ordinals is
okay but it doesn't give a good spread if you really have modules that
have everything the same except for the modifiers then it doesn't
really help.
I gave it a bit of thought. However I let the current PR to continue to
use ordinals for the hash code computation. The reason for that is this
statement on the Object.hashCode() method which says:
"the programmer should be aware that producing distinct integer results
for unequal objects may improve the performance of hash tables."
So if two module descriptors have all components equals() except for
their modifiers and if we don't use the modifiers in our hashCode()
computation, then they end up producing the same hashCode() which won't
violate the spec but will not satisfy the above statement about performance.
So I decided to stick with using the ordinal modifiers in the hashCode()
computation. However, I'm not an expert on the performance aspects of
the Collections and how much using the modifiers for hashCode() will
improve the performance in this case. Perhaps that's what you meant by
it not giving a good (enough) spread? If so, do let me know and I'll go
ahead and update the PR to remove using the modifiers in the hashCode()
computation and also update the javadoc of each of those hashCode()
methods which state that the modifiers are used in the hashCode()
computation.
-Jaikiran
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
On 03/11/21 8:50 am, Jaikiran Pai wrote:
Hello Alan,
On 02/11/21 5:30 pm, Alan Bateman wrote:
On 02/11/2021 06:38, Jaikiran Pai wrote:
:
Perhaps run 1 writing the hash code of each of the boot modules'
descriptor into a file and then run 2 reading that same file to
compare the hash codes would be one way to do it. But I think that
would just make this test more complex, which I think is avoidable.
I'm not sure that we really need this. There are several jlink tests
that check reproducibility, we think one of them is failing
intermittently with this issue already. So maybe we should leave
build reproducibility to the jlink tests and expand them as needed.
For ModuleDescriptor::hashCode then I was hoping we keep it simple
and just test that the hashCode of the descriptors for modules in the
boot layer matches the hashCode computed from re-parsing the
module-info files, e.g. something simple like this:
for (Module module : ModuleLayer.boot().modules()) {
System.out.println(module);
ModuleDescriptor descriptor1 = module.getDescriptor();
ModuleDescriptor descriptor2;
try (InputStream in =
module.getResourceAsStream("module-info.class")) {
descriptor2 = ModuleDescriptor.read(in);
}
assertEquals(descriptor1, descriptor2);
assertEquals(descriptor1.hashCode(),
descriptor2.hashCode());
assertEquals(descriptor1.compareTo(descriptor2), 0);
}
It run can twice, with with the default, the other with -Xshare:off.
Sounds reasonable. I've updated the PR to simplify the test and run it
once with default CDS and once with -Xshare:off. Given this
simplification, it now allows me to use testng for these comparisons,
so I've moved it from regular "main" to testng test case. I hope
that's OK.
Test continues to pass with these changes and fails (as expected) when
the fix in the source code of ModuleDescriptor class is rolled back.
-Jaikiran
