Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v14]
> This provides a slightly more accurate bounding limit for
> `computeNextExponentialSoftCapped`. Currently, if the maximum is specified as
> 12.0, it won't actually return a value larger than `1.5 * exponentialX0`
> (11.353912041222094); and the error gets worse as we go further into the
> tail. (This affects slightly less than 12 outputs per million for an ideal
> RNG.) This could cause the `while (computeNextExponentialSoftCapped(rng,
> limit) < limit)` check in `computeNextGaussian` on line 1402 to always be
> true, making `nextGaussian` loop infinitely in the worst case (with
> probability $$e^{-2^{63} exponentialX0} \approx 10^{-3.022 \times 10^{19}}$$
> for an ideal RNG); but more likely, it would give a result that was truncated
> too close to zero.
>
> This change is being tested prior to submission to OpenJDK by
> https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf.
Chris Hennick has updated the pull request incrementally with three additional
commits since the last revision:
- Lower limit to fix timeout
- Fix?
- Revert "Fix?"
This reverts commit 5e29b6f924269410b800c4f5a367d7bc259be5b2.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17703/files
- new: https://git.openjdk.org/jdk/pull/17703/files/080f3ea8..efc3dc4c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17703&range=13
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17703&range=12-13
Stats: 9 lines in 2 files changed: 1 ins; 0 del; 8 mod
Patch: https://git.openjdk.org/jdk/pull/17703.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17703/head:pull/17703
PR: https://git.openjdk.org/jdk/pull/17703
Negation of a regular expression in a Pattern
Dear list,
The Pattern Javadoc does not specify whether “Any character” includes
line terminators in “Any character except a, b, or c (negation)”
([^abc]) or “Any character except one in the Greek block (negation)”
(\P{InGreek}), or whether it depends on DOTALL or MULTILINE being set.
As a result, it seems to me impossible from the doc to deduce, for
example, whether the Pattern "[^a]" will match a line terminator
(spoiler alert: it does). This discussion illustrates the confusion.
Would an addition to the doc be considered?
Re: RFR: 8337279: Optimize format instant [v3]
On Sun, 28 Jul 2024 15:52:03 GMT, Shaojin Wen wrote:
>> By removing the redundant code logic in
>> DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be
>> reduced and the performance can be improved.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 1. fix handle fraction == -1
> 2. Split two methods to make codeSize less than 325
I'm still skeptical of some parts of this PR as it makes the code harder to
folow. The new tests are a good addition and should be merged.
Have you tried the performance of simply breaking out the
currentEra/beforeCurrentEra methods *without making any other changes*? Since
the logic to produce the nano fraction is going to stay in this method, I don't
really see the advantage in trying to get LocalDateTime to do the fraction some
of the time.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
3818:
> 3816: if (fractionalDigits == 0) {
> 3817: inNano = 0;
> 3818: }
Suggestion:
if (fractionalDigits == 0) {
inNano = 0;
}
boolean printNanoInLocalDateTime = fractionalDigits == -2
|| (inNano == 0 && (fractionalDigits == 0 ||
fractionalDigits == -1));
-
PR Review: https://git.openjdk.org/jdk/pull/20353#pullrequestreview-2231779313
PR Review Comment: https://git.openjdk.org/jdk/pull/20353#discussion_r1712950065
Re: RFR: 8337279: Optimize format instant [v4]
> By removing the redundant code logic in > DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be > reduced and the performance can be improved. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java Co-authored-by: Stephen Colebourne - Changes: - all: https://git.openjdk.org/jdk/pull/20353/files - new: https://git.openjdk.org/jdk/pull/20353/files/75798425..a5abb542 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20353&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20353&range=02-03 Stats: 5 lines in 1 file changed: 2 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20353.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20353/head:pull/20353 PR: https://git.openjdk.org/jdk/pull/20353
Re: RFR: 8337279: Optimize format instant [v5]
> By removing the redundant code logic in > DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be > reduced and the performance can be improved. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: copyright - Changes: - all: https://git.openjdk.org/jdk/pull/20353/files - new: https://git.openjdk.org/jdk/pull/20353/files/a5abb542..87566614 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20353&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20353&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20353.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20353/head:pull/20353 PR: https://git.openjdk.org/jdk/pull/20353
Re: RFR: 8337279: Optimize format instant [v4]
On Sun, 11 Aug 2024 10:13:12 GMT, Shaojin Wen wrote: >> By removing the redundant code logic in >> DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be >> reduced and the performance can be improved. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java > > Co-authored-by: Stephen Colebourne A more productive direction might be to move `LocalDate.formatTo` and `LocalTime.formatTo` to a JDK internal class (eg. in `jdk.internal.util`?), and then edit `LocalTime.formatTo` to handle `fractionalDigits` directly as another method parameter. This would allow the formatters in `DateTimeFormatterBuilder` to directly use the `formatTo` methods without adding any new public APIs. - PR Comment: https://git.openjdk.org/jdk/pull/20353#issuecomment-2282706467
Re: RFR: 8337832: Optimize datetime toString
On Sat, 27 Jul 2024 13:45:11 GMT, Shaojin Wen wrote: > Similar to PR #20321, this improves performance by providing a method that > passes in a StringBuilder to avoid unnecessary object allocation. Change looks fine, but is it actually faster? - Marked as reviewed by scolebourne (Author). PR Review: https://git.openjdk.org/jdk/pull/20368#pullrequestreview-2231785832
Re: RFR: 8337279: Optimize format instant [v5]
On Sun, 11 Aug 2024 10:17:08 GMT, Shaojin Wen wrote: >> By removing the redundant code logic in >> DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be >> reduced and the performance can be improved. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > copyright > I'm still skeptical of some parts of this PR as it makes the code harder to > folow. The new tests are a good addition and should be merged. > > Have you tried the performance of simply breaking out the > currentEra/beforeCurrentEra methods _without making any other changes_? Since > the logic to produce the nano fraction is going to stay in this method, I > don't really see the advantage in trying to get LocalDateTime to do the > fraction some of the time. git remote add wenshao g...@github.com:wenshao/jdk.git git fetch wenshao # baseline with test git checkout 85ee5de07dd9f45a4e1659e17f9a52e69fa77df3 make test TEST="micro:java.time.ToStringBench.instantToString" Benchmark Mode Cnt Score Error Units ToStringBench.instantToString thrpt 15 4.749 ? 0.624 ops/ms # current version git checkout 875666143e6d717634f2a3cc3c397b2ca8b63e63 make test TEST="micro:java.time.ToStringBench.instantToString" Benchmark Mode Cnt Score Error Units ToStringBench.instantToString thrpt 15 5.713 ? 0.474 ops/ms +20.29% # baseline with simply breaking out the currentEra/beforeCurrentEra methods without making any other changes git fbf66307f738cd40e061c6d26fa05c062ccd048b make test TEST="micro:java.time.ToStringBench.instantToString" Benchmark Mode Cnt Score Error Units ToStringBench.instantToString thrpt 15 4.716 ? 0.569 ops/ms -0.69% In the branch `fbf66307f738cd40e061c6d26fa05c062ccd048b`, the benchmark results are very unstable because the test data includes currentEra and beforeCurrentEra. The current version has the best performance and can handle currentEra and beforeCurrentEra The result is somewhat unexpected. I thought the performance improvement was mainly due to codeSize < 325. The specific reason needs further analysis. - PR Comment: https://git.openjdk.org/jdk/pull/20353#issuecomment-2282722011
Re: RFR: 8337832: Optimize datetime toString
On Sat, 27 Jul 2024 13:45:11 GMT, Shaojin Wen wrote: > Similar to PR #20321, this improves performance by providing a method that > passes in a StringBuilder to avoid unnecessary object allocation. Below are the performance numbers on the MacBook M1 Pro. In the zonedDateTimeToString scenario, the performance is improved by 44.81% #baseline git checkout 034297a6bd9bfcea7fa48792f54c84a6e976b319 make test TEST="micro:java.time.ToStringBench.zonedDateTimeToString" Benchmark Mode Cnt Score Error Units ToStringBench.zonedDateTimeToString thrpt 15 8.979 ? 0.206 ops/ms # current git checkout 9d7cc54c449d4e12d0eb30c103e8aa3aaf206b6d make test TEST="micro:java.time.ToStringBench.zonedDateTimeToString" Benchmark Mode Cnt Score Error Units ToStringBench.zonedDateTimeToString thrpt 15 13.003 ? 0.214 ops/ms +44.81% - PR Comment: https://git.openjdk.org/jdk/pull/20368#issuecomment-2282727944
Re: RFR: 8337279: Optimize format instant [v4]
On Sun, 11 Aug 2024 10:13:35 GMT, Stephen Colebourne wrote: > A more productive direction might be to move `LocalDate.formatTo` and > `LocalTime.formatTo` to a JDK internal class (eg. in `jdk.internal.util`?), > and then edit `LocalTime.formatTo` to handle `fractionalDigits` directly as > another method parameter. This would allow the formatters in > `DateTimeFormatterBuilder` to directly use the `formatTo` methods without > adding any new public APIs. I prefer to provide JavaTimeAccess in SharedSecrets, which will require less changes. This depends on https://github.com/openjdk/jdk/pull/20368 - PR Comment: https://git.openjdk.org/jdk/pull/20353#issuecomment-2282729464
Re: RFR: 8322256: Define and document GZIPInputStream concatenated stream semantics [v8]
On Sat, 27 Jul 2024 15:00:51 GMT, Archie Cobbs wrote: >> `GZIPInputStream` supports reading data from multiple concatenated GZIP data >> streams since [JDK-4691425](https://bugs.openjdk.org/browse/JDK-4691425). In >> order to do this, after a GZIP trailer frame is read, it attempts to read a >> GZIP header frame and, if successful, proceeds onward to decompress the new >> stream. If the attempt to decode a GZIP header frame fails, or happens to >> trigger an `IOException`, it just ignores the trailing garbage and/or the >> `IOException` and returns EOF. >> >> There are several issues with this: >> >> 1. The behaviors of (a) supporting concatenated streams and (b) ignoring >> trailing garbage are not documented, much less precisely specified. >> 2. Ignoring trailing garbage is dubious because it could easily hide errors >> or other data corruption that an application would rather be notified about. >> Moreover, the API claims that a `ZipException` will be thrown when corrupt >> data is read, but obviously that doesn't happen in the trailing garbage >> scenario (so N concatenated streams where the last one has a corrupted >> header frame is indistinguishable from N-1 valid streams). >> 3. There's no way to create a `GZIPInputStream` that does _not_ support >> stream concatenation. >> >> On the other hand, `GZIPInputStream` is an old class with lots of existing >> usage, so it's important to preserve the existing behavior, warts and all >> (note: my the definition of "existing behavior" here includes the bug fix in >> [JDK-7036144](https://bugs.openjdk.org/browse/JDK-7036144)). >> >> So this patch adds a new constructor that takes two new parameters >> `allowConcatenation` and `allowTrailingGarbage`. The legacy behavior is >> enabled by setting both to true; otherwise, they do what they sound like. In >> particular, when `allowTrailingGarbage` is false, then the underlying input >> must contain exactly one (if `allowConcatenation` is false) or exactly N (if >> `allowConcatenation` is true) concatenated GZIP data streams, otherwise an >> exception is guaranteed. > > Archie Cobbs has updated the pull request incrementally with two additional > commits since the last revision: > > - Refactor to eliminate "lenient" mode (still failng test: GZIPInZip.java). > - Add GZIP input test for various gzip(1) command outputs. Hello Archie, > That sounds reasonable. Importantly it doesn't specify the swallowing of > IOException's, which means hopefully someday we can stop doing that... > > Would you guys recommend including some kind of additional statement like > this? > If an IOException is thrown while attempting to read or decode a GZIP header > following a previous GZIP stream's trailer... We should leave out any mention of IOException from the javadoc specification. That will then make this an implementation detail and like you note, it will then allow us to decide in future how we want to deal with that specific part of the code. - PR Comment: https://git.openjdk.org/jdk/pull/18385#issuecomment-2282732551
Re: RFR: 8337279: Optimize format instant [v6]
> By removing the redundant code logic in > DateTimeFormatterBuilder$InstantPrinterParser#formatTo, the codeSize can be > reduced and the performance can be improved. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: breaking out the printNano methods - Changes: - all: https://git.openjdk.org/jdk/pull/20353/files - new: https://git.openjdk.org/jdk/pull/20353/files/87566614..ccbd6b5a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20353&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20353&range=04-05 Stats: 24 lines in 1 file changed: 14 ins; 9 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20353.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20353/head:pull/20353 PR: https://git.openjdk.org/jdk/pull/20353
Package.getPackage deprecation
I make use of Package.getPackage in Joda-Convert but the method has
now been deprecated. I'd like to update my code, but AFAICT the
suggested alternative does not work.
The Joda-Convert code allows a user to convert a Package object to a
String, and back again. Reading the deprecation, I'm aware that this
may not work perfectly, but I do have to maintain compatibility as
best I can.
The deprecation asks users to use ClassLoader#getDefinedPackage
instead. To do this properly, users have to loop up the ClassLoader
parent list. However, the boot class loader is generally returned as
null, and it is obviously impossible to call getDefinedPackage on a
null reference.
ie.
Package.getPackage("java.util") works OK, but is deprecated
The suggested replacement is this code, but it does not work because
the boot class loader is returned as null:
private static Package parsePackage(String str) {
var loader = JDKStringConverter.class.getClassLoader();
var pkg = (Package) null;
while (loader != null && pkg == null) {
pkg = loader.getDefinedPackage(str);
loader = loader.getParent();
}
return pkg;
}
Am I misunderstanding things?
See also
https://stackoverflow.com/questions/77259364/how-to-get-a-package-from-its-path-in-java
My current workaround is to call Package.getAllPackages() and search
for "java.util" manually, which appears to work OK.
thanks
Stephen
Re: Package.getPackage deprecation
On 11/08/2024 15:17, Stephen Colebourne wrote:
I make use of Package.getPackage in Joda-Convert but the method has
now been deprecated. I'd like to update my code, but AFAICT the
suggested alternative does not work.
The Joda-Convert code allows a user to convert a Package object to a
String, and back again. Reading the deprecation, I'm aware that this
may not work perfectly, but I do have to maintain compatibility as
best I can.
The deprecation asks users to use ClassLoader#getDefinedPackage
instead. To do this properly, users have to loop up the ClassLoader
parent list. However, the boot class loader is generally returned as
null, and it is obviously impossible to call getDefinedPackage on a
null reference.
ie.
Package.getPackage("java.util") works OK, but is deprecated
The suggested replacement is this code, but it does not work because
the boot class loader is returned as null:
private static Package parsePackage(String str) {
var loader = JDKStringConverter.class.getClassLoader();
var pkg = (Package) null;
while (loader != null && pkg == null) {
pkg = loader.getDefinedPackage(str);
loader = loader.getParent();
}
return pkg;
}
Am I misunderstanding things?
Package.getPackage is deprecated a long time, I don't think we've seen
too many complaints. Nowadays it's probably not too useful except to get
to package annotations (everything else in that API dates from JDK 1.2
and the since removed extension mechanism).
The set of package names for packages in the modules defined to the boot
loader can be obtained with code like this:
ModuleLayer.boot()
.modules()
.stream()
.filter(m -> m.getClassLoader() == null)
.flatMap(m -> m.getPackages().stream())
.collect(Collectors.toSet());
which I think is what you are looking for here.
-Alan
Re: RFR: 8322256: Define and document GZIPInputStream concatenated stream semantics [v9]
> `GZIPInputStream` supports reading data from multiple concatenated GZIP data > streams since [JDK-4691425](https://bugs.openjdk.org/browse/JDK-4691425). In > order to do this, after a GZIP trailer frame is read, it attempts to read a > GZIP header frame and, if successful, proceeds onward to decompress the new > stream. If the attempt to decode a GZIP header frame fails, or happens to > trigger an `IOException`, it just ignores the trailing garbage and/or the > `IOException` and returns EOF. > > There are several issues with this: > > 1. The behaviors of (a) supporting concatenated streams and (b) ignoring > trailing garbage are not documented, much less precisely specified. > 2. Ignoring trailing garbage is dubious because it could easily hide errors > or other data corruption that an application would rather be notified about. > Moreover, the API claims that a `ZipException` will be thrown when corrupt > data is read, but obviously that doesn't happen in the trailing garbage > scenario (so N concatenated streams where the last one has a corrupted header > frame is indistinguishable from N-1 valid streams). > 3. There's no way to create a `GZIPInputStream` that does _not_ support > stream concatenation. > > On the other hand, `GZIPInputStream` is an old class with lots of existing > usage, so it's important to preserve the existing behavior, warts and all > (note: my the definition of "existing behavior" here includes the bug fix in > [JDK-7036144](https://bugs.openjdk.org/browse/JDK-7036144)). > > So this patch adds a new constructor that takes two new parameters > `allowConcatenation` and `allowTrailingGarbage`. The legacy behavior is > enabled by setting both to true; otherwise, they do what they sound like. In > particular, when `allowTrailingGarbage` is false, then the underlying input > must contain exactly one (if `allowConcatenation` is false) or exactly N (if > `allowConcatenation` is true) concatenated GZIP data streams, otherwise an > exception is guaranteed. Archie Cobbs has updated the pull request incrementally with one additional commit since the last revision: Revert all functional changes, leaving only tests & Javadoc. - Changes: - all: https://git.openjdk.org/jdk/pull/18385/files - new: https://git.openjdk.org/jdk/pull/18385/files/f3939c05..a69bf2a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18385&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18385&range=07-08 Stats: 133 lines in 3 files changed: 18 ins; 84 del; 31 mod Patch: https://git.openjdk.org/jdk/pull/18385.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18385/head:pull/18385 PR: https://git.openjdk.org/jdk/pull/18385
Re: RFR: 8322256: Define and document GZIPInputStream concatenated stream semantics [v8]
On Sat, 27 Jul 2024 15:00:51 GMT, Archie Cobbs wrote: >> `GZIPInputStream` supports reading data from multiple concatenated GZIP data >> streams since [JDK-4691425](https://bugs.openjdk.org/browse/JDK-4691425). In >> order to do this, after a GZIP trailer frame is read, it attempts to read a >> GZIP header frame and, if successful, proceeds onward to decompress the new >> stream. If the attempt to decode a GZIP header frame fails, or happens to >> trigger an `IOException`, it just ignores the trailing garbage and/or the >> `IOException` and returns EOF. >> >> There are several issues with this: >> >> 1. The behaviors of (a) supporting concatenated streams and (b) ignoring >> trailing garbage are not documented, much less precisely specified. >> 2. Ignoring trailing garbage is dubious because it could easily hide errors >> or other data corruption that an application would rather be notified about. >> Moreover, the API claims that a `ZipException` will be thrown when corrupt >> data is read, but obviously that doesn't happen in the trailing garbage >> scenario (so N concatenated streams where the last one has a corrupted >> header frame is indistinguishable from N-1 valid streams). >> 3. There's no way to create a `GZIPInputStream` that does _not_ support >> stream concatenation. >> >> On the other hand, `GZIPInputStream` is an old class with lots of existing >> usage, so it's important to preserve the existing behavior, warts and all >> (note: my the definition of "existing behavior" here includes the bug fix in >> [JDK-7036144](https://bugs.openjdk.org/browse/JDK-7036144)). >> >> So this patch adds a new constructor that takes two new parameters >> `allowConcatenation` and `allowTrailingGarbage`. The legacy behavior is >> enabled by setting both to true; otherwise, they do what they sound like. In >> particular, when `allowTrailingGarbage` is false, then the underlying input >> must contain exactly one (if `allowConcatenation` is false) or exactly N (if >> `allowConcatenation` is true) concatenated GZIP data streams, otherwise an >> exception is guaranteed. > > Archie Cobbs has updated the pull request incrementally with two additional > commits since the last revision: > > - Refactor to eliminate "lenient" mode (still failng test: GZIPInZip.java). > - Add GZIP input test for various gzip(1) command outputs. OK I've reverted any functional changes. The net result should just be additional Javadoc and two new unit tests. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/18385#issuecomment-2282855270
Withdrawn: 8310843: Reimplement ByteArray and ByteArrayLittleEndian with Unsafe
On Mon, 10 Jun 2024 02:12:11 GMT, Glavo wrote: > Things have changed since https://github.com/openjdk/jdk/pull/14636 was > closed, so let me reopen it. > > https://github.com/openjdk/jdk/pull/15386 confirmed that `VarHandle` in BALE > caused a startup regression. In order to not have any more revert patches, it > makes sense to optimize BALE. > > The optimization of https://github.com/openjdk/jdk/pull/16245 allows the > traditional expression to have good performance, but BA and BALE save us from > having to copy these lengthy expressions everywhere. So it makes sense to > keep them. > > Now here's the question, should I rewrite this PR without `Unsafe`? I'll do > some research (e.g. does `Unsafe` have better performance during warmup?), > but I'd also like to take some advice. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/19616
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v14]
On Sun, 11 Aug 2024 07:30:12 GMT, Chris Hennick wrote:
>> This provides a slightly more accurate bounding limit for
>> `computeNextExponentialSoftCapped`. Currently, if the maximum is specified
>> as 12.0, it won't actually return a value larger than `1.5 * exponentialX0`
>> (11.353912041222094); and the error gets worse as we go further into the
>> tail. (This affects slightly less than 12 outputs per million for an ideal
>> RNG.) This could cause the `while (computeNextExponentialSoftCapped(rng,
>> limit) < limit)` check in `computeNextGaussian` on line 1402 to always be
>> true, making `nextGaussian` loop infinitely in the worst case (with
>> probability $$e^{-2^{63} exponentialX0} \approx 10^{-3.022 \times 10^{19}}$$
>> for an ideal RNG); but more likely, it would give a result that was
>> truncated too close to zero.
>>
>> This change is being tested prior to submission to OpenJDK by
>> https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf.
>
> Chris Hennick has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Lower limit to fix timeout
> - Fix?
> - Revert "Fix?"
>
>This reverts commit 5e29b6f924269410b800c4f5a367d7bc259be5b2.
Update: having read the McFarland paper, I'm no longer confident this bug
actually exists or that any performance improvements are possible.
-
PR Comment: https://git.openjdk.org/jdk/pull/17703#issuecomment-2282898111
RFR: 8337302: Undefined type variable results in null
When a type uses a type variable without a declaration, no exception is thrown. This change triggers a `TypeNotFoundException` to be thrown. - Commit messages: - JDK-8337302: Throw exception upon search for type variable without representation. Changes: https://git.openjdk.org/jdk/pull/20535/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20535&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337302 Stats: 84 lines in 3 files changed: 74 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/20535.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20535/head:pull/20535 PR: https://git.openjdk.org/jdk/pull/20535
Re: Package.getPackage deprecation
On Sun, 11 Aug 2024 at 17:19, Alan Bateman wrote: > Package.getPackage is deprecated a long time, I don't think we've seen too > many complaints. Nowadays it's probably not too useful except to get to > package annotations (everything else in that API dates from JDK 1.2 and the > since removed extension mechanism). > > The set of package names for packages in the modules defined to the boot > loader can be obtained with code like this: > > ModuleLayer.boot() > .modules() > .stream() > .filter(m -> m.getClassLoader() == null) > .flatMap(m -> m.getPackages().stream()) > .collect(Collectors.toSet()); > > which I think is what you are looking for here. That doesn't quite work as it returns String package names, not Package objects (I need the Package object to maintain compatibility). Sounds like Package.getPackages() is the only current workaround. FWIW I do think there is a case to review the deprecation text of Package.getPackage(String). thanks Stephen
Re: Package.getPackage deprecation
Hi Stephen, I agree: there's no way to get defined packages for the boot class loader directly via Package.getDefinedPackage. They are not accessible via the system or platform class loaders. The alternative approaches via Package.getDefinedPackages() or getPackage() can be intercepted by custom class loaders as well. Since the introduction of the Module system, package is now seen more like a property, that a package in a class loader always maps to one module. (This trick has been useful in defining classes with MethodHandles.Lookup) I think we probably need a way to work around this restriction; it indeed makes little sense that regular java code cannot access the boot loader packages reliably. I think we can do this as easy as making ClassLoader.getPackages() and getPackage() public like getDefinedPackages() or getDefinedPackage(), but the impact is unsure. P.S. About the package class itself, we can probably upgrade its nested VersionInfo class to a record, and make Package itself final. Chen Liang On Sun, Aug 11, 2024 at 5:50 PM Stephen Colebourne wrote: > On Sun, 11 Aug 2024 at 17:19, Alan Bateman > wrote: > > Package.getPackage is deprecated a long time, I don't think we've seen > too many complaints. Nowadays it's probably not too useful except to get to > package annotations (everything else in that API dates from JDK 1.2 and the > since removed extension mechanism). > > > > The set of package names for packages in the modules defined to the boot > loader can be obtained with code like this: > > > > ModuleLayer.boot() > > .modules() > > .stream() > > .filter(m -> m.getClassLoader() == null) > > .flatMap(m -> m.getPackages().stream()) > > .collect(Collectors.toSet()); > > > > which I think is what you are looking for here. > > That doesn't quite work as it returns String package names, not > Package objects (I need the Package object to maintain compatibility). > Sounds like Package.getPackages() is the only current workaround. > > FWIW I do think there is a case to review the deprecation text of > Package.getPackage(String). > > thanks > Stephen >
Re: RFR: 8310843: Reimplement ByteArray and ByteArrayLittleEndian with Unsafe [v2]
On Mon, 10 Jun 2024 03:51:40 GMT, Glavo wrote: >> Things have changed since https://github.com/openjdk/jdk/pull/14636 was >> closed, so let me reopen it. >> >> https://github.com/openjdk/jdk/pull/15386 confirmed that `VarHandle` in BALE >> caused a startup regression. In order to not have any more revert patches, >> it makes sense to optimize BALE. >> >> The optimization of https://github.com/openjdk/jdk/pull/16245 allows the >> traditional expression to have good performance, but BA and BALE save us >> from having to copy these lengthy expressions everywhere. So it makes sense >> to keep them. >> >> Now here's the question, should I rewrite this PR without `Unsafe`? I'll do >> some research (e.g. does `Unsafe` have better performance during warmup?), >> but I'd also like to take some advice. > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > update copyright With merge stores, you can probably reimplement these two classes with direct array writes instead of Unsafe, for ease of maintenance. - PR Comment: https://git.openjdk.org/jdk/pull/19616#issuecomment-2282957595
Re: RFR: 8337302: Undefined type variable results in null
On Sun, 11 Aug 2024 22:25:40 GMT, Rafael Winterhalter
wrote:
> When a type uses a type variable without a declaration, no exception is
> thrown. This change triggers a `TypeNotFoundException` to be thrown.
src/java.base/share/classes/java/lang/TypeNotPresentException.java line 37:
> 35: * this exception is unchecked.
> 36: *
> 37: * Note that this exception may be used when undefined type variables
Seems we can remove this note, now that we moved it to the last paragraph :)
test/jdk/java/lang/reflect/Generics/TestMissingTypeVariable.java line 40:
> 38:
> 39: public static void main(String[] args) throws Exception {
> 40: ClassWriter classWriter = new
> ClassWriter(ClassWriter.COMPUTE_MAXS);
We currently use ClassFile API to quickly dump classes and there's a test
infrastructure class `ByteCodeLoader` once you declare `@library /test/lib`
jtreg directive. See example here:
https://github.com/openjdk/jdk/pull/19281/files#diff-6407a097383e3239602ee0621dcf17e8a90ba2d990a4546e4ea4ff89ad9b89d5
Updated `TestBadSignatures.java`
P.S. accidentally commented on your commit instead of here as a review comment.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20535#discussion_r1713093516
PR Review Comment: https://git.openjdk.org/jdk/pull/20535#discussion_r1713094798
Re: RFR: 8329251: Print custom truststore/ keystore name [v2]
> Using SharedSecrets, I attempted to expose FileInputStream::path information. > After implementing the fix, I validated the startup performance tests. > Observed no consistent pattern of performance drops or gains, can disregard > the occasional performance drop observed in 1 or 2 runs. Prasadrao Koppula has updated the pull request incrementally with one additional commit since the last revision: JDK-8329251 - Changes: - all: https://git.openjdk.org/jdk/pull/20414/files - new: https://git.openjdk.org/jdk/pull/20414/files/22bc6e00..e23a70d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20414&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20414&range=00-01 Stats: 13 lines in 3 files changed: 1 ins; 7 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/20414.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20414/head:pull/20414 PR: https://git.openjdk.org/jdk/pull/20414
Re: RFR: 8329251: Print custom truststore/ keystore name
On Thu, 1 Aug 2024 04:11:24 GMT, Prasadrao Koppula wrote: > Using SharedSecrets, I attempted to expose FileInputStream::path information. > After implementing the fix, I validated the startup performance tests. > Observed no consistent pattern of performance drops or gains, can disregard > the occasional performance drop observed in 1 or 2 runs. Please hold on the review, I'm trying to add debug statements to TLS code. - PR Comment: https://git.openjdk.org/jdk/pull/20414#issuecomment-2283127192
Re: RFR: 8338021: Support saturating vector operators in VectorAPI [v2]
On Fri, 9 Aug 2024 03:28:53 GMT, Jasmine Karthikeyan
wrote:
>> Jatin Bhateja has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains three additional
>> commits since the last revision:
>>
>> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8338201
>> - Removed redundant comment
>> - 8338021: Support saturating vector operators in VectorAPI
>
> src/hotspot/share/opto/type.cpp line 495:
>
>> 493: TypeInt::POS1= TypeInt::make(1,max_jint, WidenMin); // Positive
>> values
>> 494: TypeInt::INT = TypeInt::make(min_jint,max_jint, WidenMax); //
>> 32-bit integers
>> 495: TypeInt::UINT= TypeInt::make(0, max_juint, WidenMin); // Unsigned
>> ints
>
> This would make an illegal type, right? Since `TypeInt` is signed using
> `max_juint` as the hi value would end up as signed -1, resulting in the type
> `0..-1`, an empty type. I wonder if there's a better way to handle this,
> since in the type system empty types are in a sense equivalent to `TOP`.
@jaskarth , its usage in existing patch is limited to [type
comparison.](https://github.com/openjdk/jdk/pull/20507/files#diff-3559dcf23b719805be5fd06fd5c1851dbd8f53e47afe6d99cba13a3de0ebc6b2R1542).
My plan is to address intrinsification of new core lib APIs, associated value
range folding optimization (since unsigned numbers have different value range
of [0, MAX_VALUE) vs signed [-MIN_VALUE/2, +MAX_VALUE/2) numbers) and
auto-vectorization in a follow up patch.
**Notes on C2 type system:**
Unlike Type::FLOAT, integral type ranges are specified using _lo and _hi value
range, these ranges are pruned using flow functions associated with each
operation IR. Constraining the value ranges allows logic pruning, e.g.
in1[TypeInt] & 0x7FFF will chop off -ve values ranges from in1, thus a
constrol structure like . `if (in1 < 0) { true_path ; } else { false_path; } `
which uses in1 as a flow condition will sweepout the true path.
C2 type system only maintains value ranges for integral types i.e. long and
int, any sub-word type which as per JVM specification has an int storage "word"
only constrains the value range of TypeInt.
A type which represent a constant value has same _hi and _lo value.
Floating point types Type::FLOAT / DOUBLE cannot maintain upper / lower value
ranges due to rounding constraints.
Thus a C2 type system maintains a separate type TypeF and TypeD which are
singletons and represent a constant value.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1713220777
