Re: RFR: 8240629: argfiles parsing broken for argfiles with comment cross 4096 bytes chunk

[Mandy Chung]

On 3/8/20 11:27 AM, Henry Jen wrote:

Thanks for the review, I updated the webrev[1] with simplified test and ensure 
other cases in boundary won’t be causing trouble by only take meaningful tokens.

This fix is more defensive and allow anchor to be ignored when it’s meaningless.

[1] http://cr.openjdk.java.net/~henryjen/jdk/8240629.1/webrev/


This patch looks okay.

Mandy

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

[naoto . sato]

Thanks, Stephen.

Updated the webrev for those two suggestions:

http://cr.openjdk.java.net/~naoto/8239836/webrev.04/

Naoto

On 3/8/20 4:22 PM, Stephen Colebourne wrote:

  standardOffsets[0] == wallOffsets[0] needs to use .equals()

Unrelated, there is a Javadoc/spec error at line 578 of the "new"
file. It should be getValidOffsets, not getOffset.

Apart from that, it looks good.

thanks
Stephen


On Wed, 4 Mar 2020 at 19:06,  wrote:


Hi Stephen,

Thank you for the detailed explanation and correcting the
implementation. I incorporated all the suggestions below, as well as
adding a new test for isFixedOffset() implementation (with some clean-up):

https://cr.openjdk.java.net/~naoto/8239836/webrev.03/

Please take a look at it.

Naoto

On 3/3/20 3:30 PM, Stephen Colebourne wrote:

I fear more changes are needed here.

1) The code is isFixedOffset() is indeed wrong, but the fix is
insufficient. The zone is fixed if baseStandardOffset=baseWallOffset
and all three other lists are empty. A fixed offset rule is the
equivalent of a ZoneOffset. See ZoneId.normalized() for the code that
assumes that.

2) The code in getOffset(Instant) is wrong, but so is the proposed
fix. The purpose of the if statement is to optimise the following few
lines which refer to savingsInstantTransitions and wallOffsets. The
code does have a bug as it should return the first wall offset.
Corrected code:
public ZoneOffset getOffset(Instant instant) {
  if (savingsInstantTransitions.length == 0) {
return wallOffsets[0];
  }
With the correct definition of isFixedOffset() it becomes apparent
that this if statement is in fact not related to the fixed offset.

Currently this test case fails (a zone in DST for all eternity):
  ZoneRules rules = ZoneRules.of(
  ZoneOffset.ofHours(1),
  ZoneOffset.ofHours(2),
  Collections.emptyList(),
  Collections.emptyList(),
  Collections.emptyList());
  assertEquals(rules.getStandardOffset(Instant.EPOCH),
ZoneOffset.ofHours(1));
  assertEquals(rules.getOffset(Instant.EPOCH),  ZoneOffset.ofHours(2));

3) The code in getStandardOffset(Instant) also has an error as it
checks the wrong array. It should check standardTransitions as that is
what is used in the following few lines. Corrected code:
public ZoneOffset getStandardOffset(Instant instant) {
  if (standardTransitions.length == 0) {
return standardOffsets[0];
  }

4) The code in getOffsetInfo(LocalDateTime dt) has a similar fault.
Since it protects savingsLocalTransitions, it should return
wallOffsets[0].

5) The code in getDaylightSavings(Instant instant) has an optimising
if statement that should refer to isFixedOffset().

6) Also, in the test.
   assertTrue(!rules.isFixedOffset());
should be
   assertFalse(rules.isFixedOffset());

The class has worked so far as every normal case has
baseStandardOffset = baseWallOffset, even though it is conceptually
valid for this not to be true.

Stephen




On Thu, 27 Feb 2020 at 20:42,  wrote:


Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8239836

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8239836/webrev.00/

It turned out that 'transitionList' is not necessarily a superset of
'standardOffsetTransitionList', as in some occasions where a standard
offset change and a savings change happen at the same time (canceling
each other), resulting in no wall time transition. This means that the
"rules" in the sample code is a valid ZoneRules instance.

However, there is an assumption in ZoneRules where no (wall time)
transition means the fixed offset zone. With that assumption, the
example rule is considered a fixed offset zone which is not correct. So
the fix proposed here is not to throw an IAE but to recognize the rule
as a valid, non-fixed offset ZoneRules instance.

Naoto

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

[Stephen Colebourne]

 standardOffsets[0] == wallOffsets[0] needs to use .equals()

Unrelated, there is a Javadoc/spec error at line 578 of the "new"
file. It should be getValidOffsets, not getOffset.

Apart from that, it looks good.

thanks
Stephen


On Wed, 4 Mar 2020 at 19:06,  wrote:
>
> Hi Stephen,
>
> Thank you for the detailed explanation and correcting the
> implementation. I incorporated all the suggestions below, as well as
> adding a new test for isFixedOffset() implementation (with some clean-up):
>
> https://cr.openjdk.java.net/~naoto/8239836/webrev.03/
>
> Please take a look at it.
>
> Naoto
>
> On 3/3/20 3:30 PM, Stephen Colebourne wrote:
> > I fear more changes are needed here.
> >
> > 1) The code is isFixedOffset() is indeed wrong, but the fix is
> > insufficient. The zone is fixed if baseStandardOffset=baseWallOffset
> > and all three other lists are empty. A fixed offset rule is the
> > equivalent of a ZoneOffset. See ZoneId.normalized() for the code that
> > assumes that.
> >
> > 2) The code in getOffset(Instant) is wrong, but so is the proposed
> > fix. The purpose of the if statement is to optimise the following few
> > lines which refer to savingsInstantTransitions and wallOffsets. The
> > code does have a bug as it should return the first wall offset.
> > Corrected code:
> >public ZoneOffset getOffset(Instant instant) {
> >  if (savingsInstantTransitions.length == 0) {
> >return wallOffsets[0];
> >  }
> > With the correct definition of isFixedOffset() it becomes apparent
> > that this if statement is in fact not related to the fixed offset.
> >
> > Currently this test case fails (a zone in DST for all eternity):
> >  ZoneRules rules = ZoneRules.of(
> >  ZoneOffset.ofHours(1),
> >  ZoneOffset.ofHours(2),
> >  Collections.emptyList(),
> >  Collections.emptyList(),
> >  Collections.emptyList());
> >  assertEquals(rules.getStandardOffset(Instant.EPOCH),
> > ZoneOffset.ofHours(1));
> >  assertEquals(rules.getOffset(Instant.EPOCH),  
> > ZoneOffset.ofHours(2));
> >
> > 3) The code in getStandardOffset(Instant) also has an error as it
> > checks the wrong array. It should check standardTransitions as that is
> > what is used in the following few lines. Corrected code:
> >public ZoneOffset getStandardOffset(Instant instant) {
> >  if (standardTransitions.length == 0) {
> >return standardOffsets[0];
> >  }
> >
> > 4) The code in getOffsetInfo(LocalDateTime dt) has a similar fault.
> > Since it protects savingsLocalTransitions, it should return
> > wallOffsets[0].
> >
> > 5) The code in getDaylightSavings(Instant instant) has an optimising
> > if statement that should refer to isFixedOffset().
> >
> > 6) Also, in the test.
> >   assertTrue(!rules.isFixedOffset());
> > should be
> >   assertFalse(rules.isFixedOffset());
> >
> > The class has worked so far as every normal case has
> > baseStandardOffset = baseWallOffset, even though it is conceptually
> > valid for this not to be true.
> >
> > Stephen
> >
> >
> >
> >
> > On Thu, 27 Feb 2020 at 20:42,  wrote:
> >>
> >> Hello,
> >>
> >> Please review the fix to the following issue:
> >>
> >> https://bugs.openjdk.java.net/browse/JDK-8239836
> >>
> >> The proposed changeset is located at:
> >>
> >> https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
> >>
> >> It turned out that 'transitionList' is not necessarily a superset of
> >> 'standardOffsetTransitionList', as in some occasions where a standard
> >> offset change and a savings change happen at the same time (canceling
> >> each other), resulting in no wall time transition. This means that the
> >> "rules" in the sample code is a valid ZoneRules instance.
> >>
> >> However, there is an assumption in ZoneRules where no (wall time)
> >> transition means the fixed offset zone. With that assumption, the
> >> example rule is considered a fixed offset zone which is not correct. So
> >> the fix proposed here is not to throw an IAE but to recognize the rule
> >> as a valid, non-fixed offset ZoneRules instance.
> >>
> >> Naoto
> >>
> >>

Re: RFR: 8240629: argfiles parsing broken for argfiles with comment cross 4096 bytes chunk

[Henry Jen]

Thanks for the review, I updated the webrev[1] with simplified test and ensure 
other cases in boundary won’t be causing trouble by only take meaningful tokens.

This fix is more defensive and allow anchor to be ignored when it’s meaningless.

[1] http://cr.openjdk.java.net/~henryjen/jdk/8240629.1/webrev/

Cheers,
Henry


> On Mar 8, 2020, at 1:54 AM, Alan Bateman  wrote:
> 
> On 06/03/2020 22:40, Henry Jen wrote:
>> Hi,
>> 
>> Please review the webrev[1] fix JDK-8240629 reported earlier by Robert.
>> 
>> http://cr.openjdk.java.net/~henryjen/jdk/8240629.0/webrev/
>> 
> The changes and test look okay.
> 
> -Alan

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Denghui Dong]

Hi Alan and Erik,

Thanks for the review.

Webrev has been updated in http://cr.openjdk.java.net/~ddong/8238665/webrev.02/

Testing: jdk/jfr, java/lang/management/ all passed

Summary: add a method in jdk.internal.misc.VM to get an unmodifiable buffer 
pool list.

Cheers,
Denghui Dong

--
From:Alan Bateman 
Send Time:2020年3月8日(星期日) 22:13
To:董登辉(卓昂) ; Erik Gahlin 
Cc:hotspot-jfr-dev ; core-libs-dev 

Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

 On 05/03/2020 15:19, Denghui Dong wrote:
Hi Alan and Erik,

 Sorry, I'm not sure if I fully understand what you mean.
 For this feature, I still think exporting jdk.internal.access to jdk.jfr is a 
good solutionsince there are
 some other packages has already exported to jdk.jfr in this module.
 I think it would be cleaner to put a helper in jdk.internal.misc so that JFR 
has access to the counters for each of the buffer pools. That would avoid 
additional plumbing when you extend it to mapped buffers. As I mentioned in one 
of the other mails, ManagementFactoryHelper.getBufferPoolMXBeans() could be 
changed to use it too. From the perspective of java.base then it's a lot nicer 
as anyone touching the buffer code doesn't need to have to concerned with 
direct access from code in java.management and jdk.jfr.

 -Alan

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Alan Bateman]

On 05/03/2020 15:19, Denghui Dong wrote:

Hi Alan and Erik,

Sorry, I'm not sure if I fully understand what you mean.
For this feature, I still think exporting jdk.internal.access to jdk.jfr is a 
good solutionsince there are
some other packages has already exported to jdk.jfr in this module.
I think it would be cleaner to put a helper in jdk.internal.misc so that 
JFR has access to the counters for each of the buffer pools. That would 
avoid additional plumbing when you extend it to mapped buffers. As I 
mentioned in one of the other mails, 
ManagementFactoryHelper.getBufferPoolMXBeans() could be changed to use 
it too. From the perspective of java.base then it's a lot nicer as 
anyone touching the buffer code doesn't need to have to concerned with 
direct access from code in java.management and jdk.jfr.


-Alan

Re: RFR: 8196334: Optimize UUID#fromString

[Claes Redestad]

Hi Peter,

sorry about defaulting to throughput measurement - while average time
has its advantages (especially on nanobenchmarks), most benchmarks are
intuitively "higher is better", so I lean that way by old habit.

Thanks for trying out your ideas, though! It's important we try out and
report the result of reasonable alternatives, even when they fall short.
I've done a number of experiments here that I should have probably made
a note of somewhere...

Thanks!

/Claes

On 2020-03-08 12:59, Peter Levart wrote:

Sorry, bash this.

I just noticed that the units used in benchmark are ops/us and that the 
best performing code is the one which is in place now.


I usually use ns/op as a unit in JMH benchmarks, so this led me to false 
belief that alternatives are better performing where in fact are worse.


Sorry for noise.

Regards, Peter


On 3/8/20 12:46 PM, Peter Levart wrote:

Hi,

When I noticed this optimization, I had some ideas how to push this 
even further, but only now had some time to try them out.


Here are some incremental improvements [1] to the already brilliant 
idea with the following JMH results (the baseline is JDK build with 
patch for 8196334 already applied):


Benchmark (size)   Mode  Cnt   Score   Error Units
UUIDBench.uuidFromString   2  thrpt    5  37.344 ± 0.257 ops/us
UUIDBench.uuidFromStringAlt1   2  thrpt    5  29.069 ± 0.137 ops/us
UUIDBench.uuidFromStringAlt2   2  thrpt    5  27.308 ± 0.204 ops/us
UUIDBench.uuidFromStringAlt3   2  thrpt    5  23.195 ± 0.885 ops/us

As can be seen, the trick was to eliminate branches (alt1, alt2) and 
to reduce the number of operations (shifts) in alt3. One last trick in 
alt3 was to use the array length in a check that is then fused with 
array index bounds check (I think) so that we get one of them for free.


What do you think? Is this good enough to warrant another change? As 
you can see, I haven't prepared the patch yet,  just benchmark.


Regards, Peter

[1] 
http://cr.openjdk.java.net/~plevart/jdk-dev/8196334_UUID.fromString/UUIDBench.java 




On 3/2/20 8:26 AM, Claes Redestad wrote:

On 2020-03-02 08:14, Alan Bateman wrote:


http://cr.openjdk.java.net/~redestad/8196334/open.02/

Looks good.


Thanks - pushed!

/Claes

Re: RFR: 8216407: java.util.UUID.fromString accepts input that does not match expected format

[Alan Bateman]

On 06/03/2020 18:15, Roger Riggs wrote:

Hi Chihiro, et.al.,

Thanks for taking a look at this issue,  however...

There has been a long history of concerns[1] about breaking existing 
applications
that depend on the loose parsing of UUIDs.  Throwing an exception 
where it did not

previously is an incompatible change.

The crucial concern about performance parsing conforming strings has 
been addressed by:


8196334 Optimize UUID#fromString 



I propose to close these as WILL-NOT-FIX: and hope that the next 
several times it gets reported

they will be closed as duplicates.

8216407   
java.util.UUID.fromString accepts input that does not match expected 
format


8165199 
UUID.fromString 
accepts wrong placements of the dashes
As you say, any tightening up of the validation after 15+ years will 
create a potential compatibility issue and might not be worth it. Maybe 
we could re-purpose JDK-8216407 to clarifying the javadoc instead? That 
would at least help with these bug reports when people are surprise that 
fromString doesn't fail.


-Alan

Re: RFR: 8196334: Optimize UUID#fromString

[Peter Levart]

Sorry, bash this.

I just noticed that the units used in benchmark are ops/us and that the 
best performing code is the one which is in place now.


I usually use ns/op as a unit in JMH benchmarks, so this led me to false 
belief that alternatives are better performing where in fact are worse.


Sorry for noise.

Regards, Peter


On 3/8/20 12:46 PM, Peter Levart wrote:

Hi,

When I noticed this optimization, I had some ideas how to push this 
even further, but only now had some time to try them out.


Here are some incremental improvements [1] to the already brilliant 
idea with the following JMH results (the baseline is JDK build with 
patch for 8196334 already applied):


Benchmark (size)   Mode  Cnt   Score   Error Units
UUIDBench.uuidFromString   2  thrpt    5  37.344 ± 0.257 ops/us
UUIDBench.uuidFromStringAlt1   2  thrpt    5  29.069 ± 0.137 ops/us
UUIDBench.uuidFromStringAlt2   2  thrpt    5  27.308 ± 0.204 ops/us
UUIDBench.uuidFromStringAlt3   2  thrpt    5  23.195 ± 0.885 ops/us

As can be seen, the trick was to eliminate branches (alt1, alt2) and 
to reduce the number of operations (shifts) in alt3. One last trick in 
alt3 was to use the array length in a check that is then fused with 
array index bounds check (I think) so that we get one of them for free.


What do you think? Is this good enough to warrant another change? As 
you can see, I haven't prepared the patch yet,  just benchmark.


Regards, Peter

[1] 
http://cr.openjdk.java.net/~plevart/jdk-dev/8196334_UUID.fromString/UUIDBench.java



On 3/2/20 8:26 AM, Claes Redestad wrote:

On 2020-03-02 08:14, Alan Bateman wrote:


http://cr.openjdk.java.net/~redestad/8196334/open.02/

Looks good.


Thanks - pushed!

/Claes

Re: RFR: 8196334: Optimize UUID#fromString

[Peter Levart]

Hi,

When I noticed this optimization, I had some ideas how to push this even 
further, but only now had some time to try them out.


Here are some incremental improvements [1] to the already brilliant idea 
with the following JMH results (the baseline is JDK build with patch for 
8196334 already applied):


Benchmark (size)   Mode  Cnt   Score   Error Units
UUIDBench.uuidFromString   2  thrpt    5  37.344 ± 0.257 ops/us
UUIDBench.uuidFromStringAlt1   2  thrpt    5  29.069 ± 0.137 ops/us
UUIDBench.uuidFromStringAlt2   2  thrpt    5  27.308 ± 0.204 ops/us
UUIDBench.uuidFromStringAlt3   2  thrpt    5  23.195 ± 0.885 ops/us

As can be seen, the trick was to eliminate branches (alt1, alt2) and to 
reduce the number of operations (shifts) in alt3. One last trick in alt3 
was to use the array length in a check that is then fused with array 
index bounds check (I think) so that we get one of them for free.


What do you think? Is this good enough to warrant another change? As you 
can see, I haven't prepared the patch yet,  just benchmark.


Regards, Peter

[1] 
http://cr.openjdk.java.net/~plevart/jdk-dev/8196334_UUID.fromString/UUIDBench.java



On 3/2/20 8:26 AM, Claes Redestad wrote:

On 2020-03-02 08:14, Alan Bateman wrote:


http://cr.openjdk.java.net/~redestad/8196334/open.02/

Looks good.


Thanks - pushed!

/Claes

Re: RFR: 8240629: argfiles parsing broken for argfiles with comment cross 4096 bytes chunk

[Alan Bateman]

On 06/03/2020 22:40, Henry Jen wrote:

Hi,

Please review the webrev[1] fix JDK-8240629 reported earlier by Robert.

http://cr.openjdk.java.net/~henryjen/jdk/8240629.0/webrev/


The changes and test look okay.

-Alan

Re: RFR: 8216407: java.util.UUID.fromString accepts input that does not match expected format

[Peter Levart]

On 3/8/20 9:28 AM, Peter Levart wrote:
I can imagine bugs are a possible outcome when a programmer doesn't 
realize that different String values can successfully map to a single 
UUID value. 


Well, this is still true even with the strict method (since 'a'...'f' is 
equivalent to 'A'...'F' in parsing the Hex nibbles). So this argument 
doesn't stand. Well then perhaps this addition is not worth it.


Regards, Peter

Re: RFR: 8216407: java.util.UUID.fromString accepts input that does not match expected format

[Peter Levart]

On 3/8/20 8:45 AM, Peter Levart wrote:

Hi Roger,

What about deprecating this method (not for removal at this time) and 
creating new method UUID.valueOf(String) or similar that would be more 
strict?


... since Andriy Plokhotnyuk and Claes Redestad have already done all 
the coding (nice work!) while optimizing the UUID.fromString method in 
[1], it is just a matter of wiring up the code to new public method like 
in [2] for example. The question is only whether this is a desirable 
addition. I think it is. I can imagine bugs are a possible outcome when 
a programmer doesn't realize that different String values can 
successfully map to a single UUID value.


Regards, Peter


[1] https://bugs.openjdk.java.net/browse/JDK-8196334
[2] 
http://cr.openjdk.java.net/~plevart/jdk-dev/8216407_UUID.valueOf/webrev.01/




Peter

On 3/6/20 7:15 PM, Roger Riggs wrote:

Hi Chihiro, et.al.,

Thanks for taking a look at this issue,  however...

There has been a long history of concerns[1] about breaking existing 
applications
that depend on the loose parsing of UUIDs.  Throwing an exception 
where it did not

previously is an incompatible change.

The crucial concern about performance parsing conforming strings has 
been addressed by:


8196334 Optimize UUID#fromString 



I propose to close these as WILL-NOT-FIX: and hope that the next 
several times it gets reported

they will be closed as duplicates.

8216407  
java.util.UUID.fromString accepts input that does not match expected 
format


8165199 
UUID.fromString 
accepts wrong placements of the dashes


Any other suggestions welcome.

Thanks, Roger

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-December/057470.html



On 3/2/20 10:39 AM, Chihiro Ito wrote:

Hi,

I tried to correct this problem.

Could you review this fix, please?

According to the RFC 4122, UUID has a fixed format. I tried to raise an
exception if a string was specified that is not suitable for this
format. Also, is there anything else I should be aware of with this 
bug?


Webrev : http://cr.openjdk.java.net/~cito/JDK-8216407/webrev.00/
JBS : https://bugs.openjdk.java.net/browse/JDK-8216407

Regards,
Chihiro