Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v2]
On Fri, 26 Nov 2021 10:50:57 GMT, Daniel Fuchs wrote:
> What testing is there for this fix?
I've just added a test modelled on LdapTimeoutTest.java. (with some whitespace
issues which I'm about to fix!)
> src/java.naming/share/classes/com/sun/jndi/ldap/LdapClientFactory.java line
> 71:
>
>> 69: throws NamingException {
>> 70: return new LdapClient(host, port, socketFactory,
>> 71: (int)timeout, readTimeout, trace, pcb);
>
> A blunt cast from `long` to `int` is a bit worrying as it could lead to
> positive values becoming negative, unless you have checks in place in the
> calling code that will ensure that the long value is never >
> Integer.MAX_VALUE? And it could also result in a large value becoming a small
> positive value.
> I'd suggest to remove the inconsistency one way or the other - or add an
> explicit check to make it obvious that this case cannot happen.
Good point. I've added a check to the case. (this actually comes from
LdapPoolManager.getLdapClient which takes an int for the connection timeout
parameter, but it makes sense to be careful)
> src/java.naming/share/classes/com/sun/jndi/ldap/pool/Pool.java line 141:
>
>> 139:
>> 140: if (!conns.grabLock(remaining)) {
>> 141: throw new NamingException("Timed out waiting for lock");
>
> Is this the appropriate exception? I see in `checkRemaining`:
>
> throw new CommunicationException(
> "Timeout exceeded while waiting for a connection: " +
> timeout + "ms");
I've changed this to a CommuncationException.
-
PR: https://git.openjdk.java.net/jdk/pull/6568
Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v2]
> This fix attemps to resolve an issue where threads can stack up on each other > while waiting to get a connection from the ldap pool to an unreachable > server. It does this by having each thread start a countdown prior to holding > the pools' lock. (which has been changed to a ReentrantLock) Once the lock > has been grabbed, the timeout is adjusted to take the waiting time into > account and the process of getting a connection from the pool or creating a > new one commences. > > Note: this fix also changes the meaning of the connection pools initSize > somewhat. In a situation where we have a large initSize and a small timeout > the first thread could actually exhaust the timeout before creating all of > its initial connections. Instead this fix simply creates a single connection > per pool-connection-request. It continues to do so for subsequent requests > regardless of whether an existing unused connection is available in the pool > until initSize is exhausted. As such it may require a CSR. Rob McKenna has updated the pull request incrementally with two additional commits since the last revision: - JDK-8277795: whitespace - Add test - Changes: - all: https://git.openjdk.java.net/jdk/pull/6568/files - new: https://git.openjdk.java.net/jdk/pull/6568/files/61a7ae5b..1b679497 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6568=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6568=00-01 Stats: 133 lines in 3 files changed: 131 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6568.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6568/head:pull/6568 PR: https://git.openjdk.java.net/jdk/pull/6568
Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v4]
On Thu, 25 Nov 2021 17:57:20 GMT, Andrew Leonard wrote:
>> test/jdk/tools/jar/JarEntryTime.java line 129:
>>
>>> 127: // Make a jar file from that directory structure with
>>> 128: // --source-date set to epoch seconds 1647302400 (15/03/2022)
>>> 129: long sourceDate = 1647302400L;
>>
>> Please consider adding a few before Epoch test values.
>
> @LanceAndersen java File times can't be before the epoch, but having a test
> before dostime 1980 would be useful
>
The change to sun/tools/jar/GNUStyleOptions.java does not prevent a negative
value which can be set via ZipEntry similar to:
public void testOfEpochSecond() {
var ze = new ZipEntry("test");
for(var i = 0; i < 100; i++) {
var time = LocalDateTime.ofEpochSecond(-i, 0, ZoneOffset.UTC);
ze.setTimeLocal(time);
System.out.printf(
"time= %s, Zip Entry time= %s%n", time, ze.getTimeLocal());
}
}
If the intent is to not support dates prior to the Epoch then GNUStyleOptions
should throw an Exception in this case.
-
PR: https://git.openjdk.java.net/jdk/pull/6481
Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v4]
On Fri, 26 Nov 2021 12:23:41 GMT, Sean Coffey wrote:
>> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 2290:
>>
>>> 2288: private void setSourceDate(ZipEntry e, long origTime) {
>>> 2289: if (sourceDate != -1) {
>>> 2290: e.setTimeLocal(LocalDateTime.ofEpochSecond(sourceDate, 0,
>>> ZoneOffset.UTC));
>>
>> The above could potentially throw a DateTimeException which may be OK but I
>> would sanity check there are no issues
>
> For files with large number of entries, alot of LocalDateTime Objects are
> being created here. Would there be an efficiency gain by converting
> sourceDate variable to be of type LocalDateTime and simply pass it to the
> s.setTimeLocal call here. The LocalDateTime Object could be constructed at
> init time (may be null) and can be static etc.
>
> Same for jmod code ?
>
I think that is a reasonable suggestion
-
PR: https://git.openjdk.java.net/jdk/pull/6481
Re: RFR: 8277868: Use Comparable.compare() instead of surrogate code
On Fri, 26 Nov 2021 12:46:59 GMT, Сергей Цыпанов wrote: > Instead of something like > > long x; > long y; > return (x < y) ? -1 : ((x == y) ? 0 : 1); > > we can use `return Long.compare(x, y);` > > All replacements are done with IDE. Changes to java.net look good. Please obtain approval from reviewers in the other areas before integrating. - PR: https://git.openjdk.java.net/jdk/pull/6575
Re: Adding an @Immutable annotation to Java
If this is really something that's desired, then why use an annotation
when there's a keyword already available: const. However, there's a
reason that's never been actually used in Java, and that's because it's
so hard to get right.
On 26/11/2021 00:11, Alan Snyder wrote:
I like the idea of an @Immutable annotation. So much so, that I have created
one and use it heavily.
However, my @Immutable is just for documentation; it does not have a rigorous
semantics that would be
needed for a compiler to validate its use or generate different code.
But it could be used by lint-like tools to warn against suspicious code.
My @Immutable applies only to classes and interfaces, not variables or type
parameters.
A class or interface is either @Immutable or it is not.
That avoids much complexity.
I think it is fine to have an @Immutable list (type) whose elements are mutable.
Is an instance of an immutable list with mutable elements immutable or not?
Tell me why you want to know and I might be able to answer the question.
I also think there are cases where an @Immutable type might have a mutable
implementation
(in other words, it might have instance variables that are not final or have
non-immutable types).
Examples include internal caching and configurability of performance or
incidental behavior such as logging.
In general, immutability is in the mind of the beholder.
A case can be made that an interface or class that is @Immutable should only
permit
extensions and implementing classes that are @Immutable.
Perhaps a compiler could validate that, but only at compilation time
(to ensure binary compatibility when an @Immutable annotation is added to an
existing class or interface).
The idea of the compiler trying to enforce a semantics of immutability is scary.
Java tried to do this with uninitialized and final instance variables, and the
result has been a disaster.
The rules make semantically valid code illegal, forcing complex workarounds.
(For example, suppose you want to compute a value, bind it to a final instance
variable, and pass it to the superclass constructor.)
Yet it is still possible, actually quite easy, to write code that accesses
uninitialized final variables.
I have code that checks a final @NotNull instance variable to ensure that it is
not null.
IDEA tells me the check is unnecessary, but IDEA is wrong.
Immutability is a much more complicated concept than uninitialized.
Not something the compiler should mess with.
Alan
On Nov 25, 2021, at 10:10 AM, Ralph Goers wrote:
I would think that if a class is marked immutable that would imply all the
fields in it and from its inherited
classes should be immutable. If fields are marked immutable then it would only
apply to them.
What I wonder is what its relationship to final would be. The final annotation
implies that a field must be
set in the constructor and cannot be modified after that. I would imagine that
@immutable to have to imply
@final but would also apply at runtime. For example, where declaring a List
field as final means you cannot
replace the List once it is set. I would expect @Immutable to do the same but
also mean that you cannot
add elements to the List through that reference. But that would also mean that
you cannot pass the reference
to another variable that isn’t also annotated with @Immutable - unless the
immutable attribute becomes some
kind of internal flag on the object.
To be clear, this concept is always something I have wanted in Java. It is a
real pain to have to do things like
List list = Collections.unmodifiableList(List.of(“foo”, “bar”));
Instead, it would be nice to be able to do
@Immutable List list = List.of(“foo”, “bar”);
Although the two could be implemented to do the same thing, the second could
prevent passing the field in a
parameter that wasn’t declared @Immutable. Likewise, passing a non-immutable
list in a parameter annotated
with @Immutable could cause the list to be copied to an immutable list
automatically.
Ralph
On Nov 25, 2021, at 2:49 AM, Mariell Hoversholm
wrote:
On Thu, 25 Nov 2021 at 10:03, Andrew Haley
wrote:
Quick question, out of curiosity: how would it behave with respect to
inheritance? Can a @Immutable class inherit from an non immutable one?
And: does @Immutable mean deeply immutable? IMO it really should,
but that's harder to check, and we'd have to think about what this
means for binary compatibility.
As cited in the original email,
and the programmer could, for example, annotate a new record object with
@Immutable only if all its fields are annotated with @Immutable.
I would infer from this that it would mean deeply immutable.
To clarify further, the following record `Wrapper` would be legal only
because `A` has `@Immutable` on its _type_:
@Immutable class A {}
@Immutable record Wrapper(A a) {}
while this would not be legal:
class A {}
@Immutable record Wrapper(A a) {}
because `A` is not `@Immutable`. This could
Re: String.indexOf(single-char-String)
added benchmark results for OpenJDK's StringIndexOf benchmark:
https://github.com/openjdk/jdk/pull/6509#issuecomment-979985594
-michael
On 25.11.21 15:05, Michael Bien wrote:
Hello again,
I was trying to run JDK's benchmarks over night (second attempt
actually) but had some difficulties to get stable results.
This makes it difficult to compare the modified version with a
reference. I am not sure what the cause is, I have heard some intel
CPUs can't run avx instructions for a long time without changing clock
- maybe i am hitting this issue?
Its not the temperature and i turned boost and HT off + it runs in
headless mode. One run already takes almost 5h and I have to run it
twice - so i can't increase the iterations even more.
for example:
# Benchmark:
org.openjdk.bench.java.lang.StringIndexOfChar.utf16_mixed_String
# Parameters: (loops = 10, pathCnt = 1000, rngSeed = 1999)
# Run progress: 95.35% complete, ETA 00:13:20
# Fork: 1 of 1
# Warmup Iteration 1: 18592.094 ns/op <- second fastest run?
# Warmup Iteration 2: 20519.413 ns/op
# Warmup Iteration 3: 19768.099 ns/op
# Warmup Iteration 4: 23093.410 ns/op
# Warmup Iteration 5: 29112.909 ns/op
# Warmup Iteration 6: 18962.671 ns/op
# Warmup Iteration 7: 16721.933 ns/op <- fastest run?
# Warmup Iteration 8: 20267.809 ns/op
# Warmup Iteration 9: 23934.031 ns/op
# Warmup Iteration 10: 22474.836 ns/op
# Warmup Iteration 11: 19583.471 ns/op
# Warmup Iteration 12: 19595.319 ns/op
# Warmup Iteration 13: 24865.299 ns/op
# Warmup Iteration 14: 19581.014 ns/op
# Warmup Iteration 15: 19566.849 ns/op
# Warmup Iteration 16: 19576.219 ns/op
# Warmup Iteration 17: 19574.475 ns/op
# Warmup Iteration 18: 19565.854 ns/op
# Warmup Iteration 19: 26594.867 ns/op
# Warmup Iteration 20: 26532.977 ns/op
Iteration 1: 25484.070 ns/op
Iteration 2: 19594.206 ns/op
Iteration 3: 30327.037 ns/op
Iteration 4: 31029.242 ns/op <- xxx
Iteration 5: 19560.472 ns/op
Iteration 6: 19611.728 ns/op
Iteration 7: 23214.511 ns/op
Iteration 8: 28455.757 ns/op
Iteration 9: 19787.638 ns/op
Iteration 10: 23737.501 ns/op
Iteration 11: 25947.249 ns/op
Iteration 12: 19768.214 ns/op
Iteration 13: 25789.970 ns/op
Iteration 14: 20558.622 ns/op
Iteration 15: 19611.317 ns/op
Iteration 16: 27761.431 ns/op
Iteration 17: 19749.799 ns/op
Iteration 18: 20862.478 ns/op
Iteration 19: 19581.498 ns/op
Iteration 20: 28094.839 ns/op
latin1_Short_String, latin1_Short_char, latin1_mixed_String,
latin1_mixed_char, utf16_mixed_String and utf16_mixed_char have all
large error bars (all in StringIndexOfChar).
best regards,
michael
On 23.11.21 17:06, Michael Bien wrote:
On 23.11.21 15:57, Roger Riggs wrote:
Hi Michael,
As you might expect performance of strings is very sensitive and has
been tuned extensively over the years many times.
Though this improves the performance for 1 character strings. It
will have an impact on *every other* length of string.
You'll need to show that it does not impact performance of longer
strings.
yes of course. The if (str.length == 1) branch should be dead code
and eliminated by the JVM for all String constants with non-one lengths.
Looking through the benchmarks in micro/*/java/lang/String*, all seem
to be using constants as parameter for indexOf(). To try to measure
the impact of the if branch i would have to write a benchmark with a
parameter which changes every iteration, right? Otherwise the branch
will be optimized away by the JIT.
It may be worth looking further at other ways to achieve the result.
agreed, I tried the most obvious approach first, but there is a
chance that the fast path can be put into the intrinsified
StringLatin1/StringUTF16 code instead.
-michael
Regards, Roger
On 11/22/21 3:52 PM, Michael Bien wrote:
Hello,
I kept forgetting which variants of the String methods perform
better with single-char-Strings and which with char (IDEs had the
tendency to suggest the wrong variant since it changed between JDK
releases). So i wrote JMH benchmarks and noticed that the last
method with a performance difference seems to be String.indexOf() -
all other variants performed equally (unless I overlooked some).
this might be fairly easy to fix:
https://github.com/openjdk/jdk/pull/6509
(side effect: contains("c") is also faster)
I haven't looked into the intrinsified code of StringLatin1 and
StringUTF16 to check if it could be fixed there (mostly because i
actually don't know how the JVM assembles those intrinsics). It
might be possible to improve this for short Strings in general, not
just for chars, dependent on why the intrinsified version is
actually slower for single-char-Strings. I opted for the trivial
fix in java code.
best regards,
michael
RFR: 8277868: Use Comparable.compare() instead of surrogate code
Instead of something like long x; long y; return (x < y) ? -1 : ((x == y) ? 0 : 1); we can use `return Long.compare(x, y);` All replacements are done with IDE. - Commit messages: - 8277868: Use Comparable.compare() instead of surrogate code Changes: https://git.openjdk.java.net/jdk/pull/6575/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6575=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277868 Stats: 70 lines in 12 files changed: 2 ins; 44 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/6575.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6575/head:pull/6575 PR: https://git.openjdk.java.net/jdk/pull/6575
Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v4]
On Wed, 24 Nov 2021 16:56:35 GMT, Lance Andersen wrote:
>> Andrew Leonard has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains five commits:
>>
>> - 8276766: Enable jar and jmod to produce deterministic timestamped content
>>
>>Signed-off-by: Andrew Leonard
>> - 8276766: Enable jar and jmod to produce deterministic timestamped content
>>
>>Signed-off-by: Andrew Leonard
>> - 8276766: Enable jar and jmod to produce deterministic timestamped content
>>
>>Signed-off-by: Andrew Leonard
>> - 8276766: Enable jar and jmod to produce deterministic timestamped content
>>
>>Signed-off-by: Andrew Leonard
>> - 8276766: Enable jar and jmod to produce deterministic timestamped content
>>
>>Signed-off-by: Andrew Leonard
>
> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 2290:
>
>> 2288: private void setSourceDate(ZipEntry e, long origTime) {
>> 2289: if (sourceDate != -1) {
>> 2290: e.setTimeLocal(LocalDateTime.ofEpochSecond(sourceDate, 0,
>> ZoneOffset.UTC));
>
> The above could potentially throw a DateTimeException which may be OK but I
> would sanity check there are no issues
For files with large number of entries, alot of LocalDateTime Objects are being
created here. Would there be an efficiency gain by converting sourceDate
variable to be of type LocalDateTime and simply pass it to the s.setTimeLocal
call here. The LocalDateTime Object could be constructed at init time (may be
null) and can be static etc.
Same for jmod code ?
-
PR: https://git.openjdk.java.net/jdk/pull/6481
Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention
On Thu, 25 Nov 2021 23:54:18 GMT, Rob McKenna wrote:
> This fix attemps to resolve an issue where threads can stack up on each other
> while waiting to get a connection from the ldap pool to an unreachable
> server. It does this by having each thread start a countdown prior to holding
> the pools' lock. (which has been changed to a ReentrantLock) Once the lock
> has been grabbed, the timeout is adjusted to take the waiting time into
> account and the process of getting a connection from the pool or creating a
> new one commences.
>
> Note: this fix also changes the meaning of the connection pools initSize
> somewhat. In a situation where we have a large initSize and a small timeout
> the first thread could actually exhaust the timeout before creating all of
> its initial connections. Instead this fix simply creates a single connection
> per pool-connection-request. It continues to do so for subsequent requests
> regardless of whether an existing unused connection is available in the pool
> until initSize is exhausted. As such it may require a CSR.
What testing is there for this fix?
src/java.naming/share/classes/com/sun/jndi/ldap/LdapClientFactory.java line 71:
> 69: throws NamingException {
> 70: return new LdapClient(host, port, socketFactory,
> 71: (int)timeout, readTimeout, trace, pcb);
A blunt cast from `long` to `int` is a bit worrying as it could lead to
positive values becoming negative, unless you have checks in place in the
calling code that will ensure that the long value is never > Integer.MAX_VALUE?
And it could also result in a large value becoming a small positive value.
I'd suggest to remove the inconsistency one way or the other - or add an
explicit check to make it obvious that this case cannot happen.
src/java.naming/share/classes/com/sun/jndi/ldap/pool/Pool.java line 141:
> 139:
> 140: if (!conns.grabLock(remaining)) {
> 141: throw new NamingException("Timed out waiting for lock");
Is this the appropriate exception? I see in `checkRemaining`:
throw new CommunicationException(
"Timeout exceeded while waiting for a connection: " +
timeout + "ms");
-
PR: https://git.openjdk.java.net/jdk/pull/6568
Integrated: 8277659: [TESTBUG] Microbenchmark ThreadOnSpinWaitProducerConsumer.java hangs
On Thu, 25 Nov 2021 11:57:52 GMT, Stuart Monteith wrote: > Fix java/lang/ThreadOnSpinWaitProducerConsumer by waiting for consumer thread > to finish before restarting trial method. This pull request has now been integrated. Changeset: 3383c0dc Author:Stuart Monteith Committer: Nick Gasson URL: https://git.openjdk.java.net/jdk/commit/3383c0dcc016715dcb350b6ba196a7cdc833cdc6 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8277659: [TESTBUG] Microbenchmark ThreadOnSpinWaitProducerConsumer.java hangs Reviewed-by: njian, ngasson - PR: https://git.openjdk.java.net/jdk/pull/6560
