Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Aleksey Shipilev]

Hi Brian,

On 09/23/2014 02:34 AM, Brian Burkhalter wrote:
> I created an alternate webrev using compile-time constants per your
> suggestion:
> 
> http://cr.openjdk.java.net/~bpb/4477961/webrev.01/
> 

Ah, sorry for confusing language about "compile-time constants", I meant
"compile-time constant expression", as per JLS 15.28. Constant
expressions are FP-strict, so it should be just fine correctness- and
performance-wise, and less cryptic:
 private static final double DEGREES_TO_RADIANS = PI / 180.0;

Thanks,
-Aleksey.

Re: Urgent [9] RFR (S) : JDK-8039915 NumberFormat.format() does not consider required no. of fraction digits properly

[Joe Darcy]

Hello,

I've looked over the proposed changeset as well.

I don't see a problem with the code, but I'm not (yet) convinced it is 
right.


For future work, I think be clearer to combine the CEILING and FLOOR 
cases to share a loop with a condition test based on CEILING/FLOOR lie. 
In the test, again for future work, I think it would be clearer to 
create a custom class to represent the tuple of information needed for a 
test case as opposed to spreading that information about over a set of 
parallel arrays.


For the new work in question, it might be clearer to me if the HALF_UP 
and HALF_DOWN cases were combined into a single block since they share 
much of the logic. The unique logic for each mode would be easier to see 
if the differences were placed together.


Thanks,

-Joe

On 09/22/2014 08:50 AM, Brian Burkhalter wrote:

Hi Olivier,

On Sep 22, 2014, at 7:56 AM, olivier.lagn...@oracle.com wrote:


and 2) use braces around all the statements contained in if/else blocks (see 
below). Comment #2 is nit-picky.

I tried to keep the same flavour of writing as in HALF_DOWN and HALF_EVEN 
cases, i.e. don't use brace for
terminal/leaf return true/false statements. This is not the standard however, 
at least in this file.
Will use braces in all case (i.e. the 3 of HALF_UP,  HALF_DOWN and HALF_EVEN).

I did not look at the other case. If your formatting matches the rest of the 
file then I think it is OK to leave it as-is.


Lastly and this is not really part of your changeset, but I found it curious 
that the test prints the details of all cases that succeed as opposed to those 
that fail. I would think that either all results or the failures alone ought to 
be printed instead of successes only. See for example the partial diff below 
the DigitList diff.

Since these are most often corner and tricky test cases I think it interesting 
to have the details of each result,
including infos of both why returned result is correct or wrong.
That can help the reader to understand all these tricky cases.
The bad side of it being that it prints a lot of text, with failure cases 
(hoepfully few) lost in the middle of it,
thus making failures possibly not immediate to detect.

Here is an example of what is printed in case of failure:
"

***Error formatting double value from string : 0.6868d
NumberFormat pattern is  : #,##0.###
Maximum number of fractional digits : 3
Fractional rounding digit : 4
Position of value relative to tie : above
Rounding Mode : HALF_UP
BigDecimal output : 0.68676607158436745521612465381622314453125
FloatingDecimal output : 0.6868
Error. Formatted result different from expected.
Expected output is : "0.687"
Formated output is : "0.686"


I missed that output: I was looking for the word “failure.”


There is also a reminder of the number of errors at the end of the report:
"
==> 4 tests failed

Test failed with 4 formating error(s).
"

May be providing a reminder (value + rounding-mode + rounding position)
of the failure cases at the end of the would be better ?
Like:
"
Test failed with 4 formating error(s) for following cases :
- 0.3126d, HALF_UP rounding, 3 fractional digits
- 0.6868d, HALF_UP rounding, 3 fractional digits
- 1.798876d, HALF_UP rounding, 5 fractional digits
- 1.796889d, HALF_UP rounding, 5 fractional digits
"

Would doing so be ok ?

If the test is already printing out the information you showed above (“Error 
formatting …”) then I think it is enough but the verbiage should perhaps match 
the reminder, e.g., “Failure: Error formatting double …”

Thanks,

Brian

java.time.Clock$TickClock wrong javadoc

[Steven Schlansker]

Hi core-libs-dev,

Quick note that it seems that the Javadoc for Clock$TickClock has copypasta 
from Clock$OffsetClock.
Not a huge deal for a non-public class but probably worth fixing.

http://hg.openjdk.java.net/jdk9/client/jdk/file/5edbebb72540/src/java.base/share/classes/java/time/Clock.java

/**
 * Implementation of a clock that adds an offset to an underlying clock.
 */
static final class OffsetClock extends Clock implements Serializable {

/**
 * Implementation of a clock that adds an offset to an underlying clock.
 */
static final class TickClock extends Clock implements Serializable {

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

Hi  Mike,

Thanks for the review.

For the sake of completeness I tested converting 89.0 * Double.MIN_VALUE to 
radians and Double.MAX_VALUE / 89.0 to degrees. The old version gives 0.0 and 
Double.POSITIVE_INFINITY, respectively, whereas the webrev.01 version gives the 
respective results 1.0E-323 and 1.1573059492949775E308.

Brian

On Sep 22, 2014, at 4:24 PM, Mike Duigou  wrote:

> Looks fine to me!
> 
> Mike
> 
> On Sep 22 2014, at 15:34 , Brian Burkhalter  
> wrote:
> 
>> Hi Aleksey,
>> 
>> On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev  
>> wrote:
>> 
>>> Hm, and this compiler transformation works in strictfp context? I hope
>>> compilers do the constant folding in strictfp/fdlibm mode…
>> 
>> Yes.
>> 
>>> I would probably just go and declare the private compile-time constants.
>>> This is safer, since: a) you are not at the mercy of optimizing compiler
>>> anymore (have you tried the benchmark with C1?); b) the initializing
>>> expressions are FP-strict, less opportunity for hard to diagnose numeric
>>> problem.
>> 
>> I created an alternate webrev using compile-time constants per your 
>> suggestion:
>> 
>> http://cr.openjdk.java.net/~bpb/4477961/webrev.01/
>> 
>> The performance improvement is similar to that cited for webrev.00.
>> 
>> If this version is preferable it will need approval from a JDK 9 Reviewer, 
>> of course.
>> 
>> Thanks,
>> 
>> Brian
> 

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Mike Duigou]

Looks fine to me!

Mike

On Sep 22 2014, at 15:34 , Brian Burkhalter  wrote:

> Hi Aleksey,
> 
> On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev  
> wrote:
> 
>> Hm, and this compiler transformation works in strictfp context? I hope
>> compilers do the constant folding in strictfp/fdlibm mode…
> 
> Yes.
> 
>> I would probably just go and declare the private compile-time constants.
>> This is safer, since: a) you are not at the mercy of optimizing compiler
>> anymore (have you tried the benchmark with C1?); b) the initializing
>> expressions are FP-strict, less opportunity for hard to diagnose numeric
>> problem.
> 
> I created an alternate webrev using compile-time constants per your 
> suggestion:
> 
> http://cr.openjdk.java.net/~bpb/4477961/webrev.01/
> 
> The performance improvement is similar to that cited for webrev.00.
> 
> If this version is preferable it will need approval from a JDK 9 Reviewer, of 
> course.
> 
> Thanks,
> 
> Brian

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

Hi Aleksey,

On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev  
wrote:

> Hm, and this compiler transformation works in strictfp context? I hope
> compilers do the constant folding in strictfp/fdlibm mode…

Yes.

> I would probably just go and declare the private compile-time constants.
> This is safer, since: a) you are not at the mercy of optimizing compiler
> anymore (have you tried the benchmark with C1?); b) the initializing
> expressions are FP-strict, less opportunity for hard to diagnose numeric
> problem.

I created an alternate webrev using compile-time constants per your suggestion:

http://cr.openjdk.java.net/~bpb/4477961/webrev.01/

The performance improvement is similar to that cited for webrev.00.

If this version is preferable it will need approval from a JDK 9 Reviewer, of 
course.

Thanks,

Brian

Re: RFR: 8058887: (fmt) Improve java/util/Formatter test coverage of group separators and width

[Claes Redestad]

Fair enough! I removed the Spp changes and and regenerated the test 
files using the original script:


http://cr.openjdk.java.net/~redestad/8058887/webrev.04/

Thanks!

/Claes

On 2014-09-22 23:00, Xueming Shen wrote:
It was on purpose to keep those blank lines when I wrote the Spp to 
replace the original
script. it serves the purpose of preserving  the line number the 
generated source code
(identical to the ln# of the same code line in template file). This is 
convenient/import
when debugging those generated nio source files with a specific line 
number, from

exception stack trace, for example.

-sherman

On 09/22/2014 12:02 PM, Claes Redestad wrote:

Hi,

previous attempt was considered too trivial a fix, so how about 
actually improving the test coverage, fixing the link issue in 
genBasic and upgrade Spp.java to not add thousands of empty lines 
while we're at it? Rejoice!


webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.03/

Most relevant files to review are:

http://cr.openjdk.java.net/~redestad/8058887/webrev.03/test/java/util/Formatter/Basic-X.java.template.udiff.html 

http://cr.openjdk.java.net/~redestad/8058887/webrev.03/make/src/classes/build/tools/spp/Spp.java.udiff.html 

http://cr.openjdk.java.net/~redestad/8058887/webrev.03/test/java/util/Formatter/genBasic.sh.udiff.html 



... while the others are generated from the update Spp.java tool 
using the fixed genBasic.sh.


/Claes

On 09/22/2014 04:35 PM, Claes Redestad wrote:

Hi,

can I have a review and sponsor for this trivial fix to make the 
jtreg test generator script jdk/test/java/util/Formatter/genBasic.sh 
work in the new build system?


bug: https://bugs.openjdk.java.net/browse/JDK-8058887
webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.01/

/Claes

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

Hi Mike,

It’s definitely worth mentioning and something which should be taken into 
consideration when considering the change.

Thanks,

Brian

On Sep 22, 2014, at 2:48 PM, Mike Duigou  wrote:

> I thought it was worth mentioning because I had had to deal with the 
> underflow issue in the mapping software for the Audi/Stanford autonomous. For 
> various reasons that system ends up with many very-tiny-but-non-zero 
> quantities in it's mapping and heading component and I initially had trouble 
> matching results against the Matlab implementation. Since I had to also 
> reduce quantities to -π < x <= π and -180 < x <= 180 I combined the reduce 
> and convert using an approach similar to this revised implementation.

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

Hi Aleksey,

On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev  
wrote:

> I would probably just go and declare the private compile-time constants.
> This is safer, since: a) you are not at the mercy of optimizing compiler
> anymore (have you tried the benchmark with C1?); b) the initializing
> expressions are FP-strict, less opportunity for hard to diagnose numeric
> problem.

I actually tested with compile-time constants and the result was the same as 
for what I posted. Your suggestion is a good one however so I will update and 
repost.

Thanks,

Brian

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Mike Duigou]

Hi Brian;

I thought it was worth mentioning because I had had to deal with the underflow 
issue in the mapping software for the Audi/Stanford autonomous. For various 
reasons that system ends up with many very-tiny-but-non-zero quantities in it's 
mapping and heading component and I initially had trouble matching results 
against the Matlab implementation. Since I had to also reduce quantities to -π 
< x <= π and -180 < x <= 180 I combined the reduce and convert using an 
approach similar to this revised implementation.

Mike


On Sep 22 2014, at 14:41 , Brian Burkhalter  wrote:

> Indeed these considerations made me a little nervous about the change. For 
> the edge cases which would have previously overflowed or underflowed this 
> does not seem like a problem, i.e., to obtain a valid result where one would 
> not have done before. For the cases in between however I suppose that there 
> will be some numerical inconsistencies between the two versions.
> 
> Brian
> 
> On Sep 22, 2014, at 2:24 PM, Mike Duigou  wrote:
> 
>> Looks fine.
>> 
>> I think it is always important note when a change may result in different 
>> results for some inputs. I will reiterate for the record what's mentioned in 
>> the bug:
>> 
>>> However, one caveat is that this may affect the results of some 
>>> calculations.
>>> For example, some range of numbers that used to overflow to infinity by
>>> performing the multiplication by 180, will now not overflow and will return 
>>> a
>>> valid result.
>> 
>> This also applies to very small quantities in toRadians where dividing by 
>> 180 may have previously resulted in a zero.
> 

Re: RFR [8058875]: CharsetEncoder.maxBytesPerChar() should return 4 for UTF-8

[Martin Buchholz]

Much of the documentation (especially the early stuff when supplementary
characters were rarer/nonexistent) doesn't distinguish between "character
(codepoint)" and "char" clearly enough.  Fixing that in all the docs would
be a fine thing to do.

On Mon, Sep 22, 2014 at 2:34 PM, Mark Thomas  wrote:

> On 22/09/2014 22:23, Martin Buchholz wrote:
> > I think you are mistaken. It's maxBytesPerChar, not maxBytesPerCodepoint!
>
> You are going to have to explain that some more. The Javadoc for
> CharsetEncoder.maxBytesPerChar() is explicit:
> 
> Returns the maximum number of bytes that will be produced for each
> character of input.
> 
>
> For UTF-8 that number is 4, not 3. A quick look at the source for the
> default UTF-8 encoder confirms that there are cases where it will output
> 4 bytes for a single input character.
>
> Mark
>
>
> >
> >
> > changeset:   3116:b44704ce8a08
> > user:sherman
> > date:2010-11-19 12:58 -0800
> > 6957230: CharsetEncoder.maxBytesPerChar() reports 4 for UTF-8; should be
> 3
> > Summary: changged utf-8's CharsetEncoder.maxBytesPerChar to 3
> > Reviewed-by: alanb
> >
> >
> > On Mon, Sep 22, 2014 at 1:14 PM, Ivan Gerasimov <
> ivan.gerasi...@oracle.com>
> > wrote:
> >
> >> Hello!
> >>
> >> The UTF-8 encoding allows characters that are 4 bytes long.
> >> However, CharsetEncoder.maxBytesPerChar() currently returns 3.0, which
> is
> >> not always enough.
> >>
> >> Would you please review the simple fix for this issue?
> >>
> >> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8058875
> >> WEBREV: http://cr.openjdk.java.net/~igerasim/8058875/0/webrev/
> >>
> >> Sincerely yours,
> >> Ivan
> >>
>
>

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Aleksey Shipilev]

Hi Brian,

Looks OK.

On 09/23/2014 01:10 AM, Brian Burkhalter wrote:
> Summary: Change constructs like “a * B / C” and “u / V * W” to “x *
> (Y / Z)” where lower case denotes a variable and upper case a
> constant. This forces the constant value (Y / Z) to be evaluated only
> once per VM instance, and replaces division of the variable with
> multiplication. The resulting performance improvement is more than
> 300% as measure by JMH on a MacBookPro11,1 dual core i7 running Mac
> OS 10.9.5.

Hm, and this compiler transformation works in strictfp context? I hope
compilers do the constant folding in strictfp/fdlibm mode...

I would probably just go and declare the private compile-time constants.
This is safer, since: a) you are not at the mercy of optimizing compiler
anymore (have you tried the benchmark with C1?); b) the initializing
expressions are FP-strict, less opportunity for hard to diagnose numeric
problem.

-Aleksey.

Re: RFR [8058875]: CharsetEncoder.maxBytesPerChar() should return 4 for UTF-8

[Mark Thomas]

On 22/09/2014 22:46, Xueming Shen wrote:
> On 09/22/2014 01:14 PM, Ivan Gerasimov wrote:
>> Hello!
>>
>> The UTF-8 encoding allows characters that are 4 bytes long.
>> However, CharsetEncoder.maxBytesPerChar() currently returns 3.0, which
>> is not always enough.
>>
>> Would you please review the simple fix for this issue?
>>
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8058875
>> WEBREV: http://cr.openjdk.java.net/~igerasim/8058875/0/webrev/
>>
>> Sincerely yours,
>> Ivan
> 
> The "character" in the nio Charset and CharDe/Encoder is specified as
> "sixteen-bit Unicode
> code unit", so it is reasonable to interpret the "character" in the
> "maximum number of bytes
> that will be produced for each character of input" to be the Java "char"
> as well. In case of
> UTF8, each 4-byte form supplementary character is always coded into 2
> surrogate chars,
> it's "2 byte per char". Do we have a real escalation that complains
> about this?

Ah. Got it. I see now. There are single chars that will result in 3
bytes of output but to get 4 bytes of output requires 2 chars of input.

In which case the current value of 3.0 makes sense.

Cheers,

Mark

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

Indeed these considerations made me a little nervous about the change. For the 
edge cases which would have previously overflowed or underflowed this does not 
seem like a problem, i.e., to obtain a valid result where one would not have 
done before. For the cases in between however I suppose that there will be some 
numerical inconsistencies between the two versions.

Brian

On Sep 22, 2014, at 2:24 PM, Mike Duigou  wrote:

> Looks fine.
> 
> I think it is always important note when a change may result in different 
> results for some inputs. I will reiterate for the record what's mentioned in 
> the bug:
> 
>> However, one caveat is that this may affect the results of some calculations.
>> For example, some range of numbers that used to overflow to infinity by
>> performing the multiplication by 180, will now not overflow and will return a
>> valid result.
> 
> This also applies to very small quantities in toRadians where dividing by 180 
> may have previously resulted in a zero.

Re: RFR [8058875]: CharsetEncoder.maxBytesPerChar() should return 4 for UTF-8

[Mark Thomas]

On 22/09/2014 22:23, Martin Buchholz wrote:
> I think you are mistaken. It's maxBytesPerChar, not maxBytesPerCodepoint!

You are going to have to explain that some more. The Javadoc for
CharsetEncoder.maxBytesPerChar() is explicit:

Returns the maximum number of bytes that will be produced for each
character of input.


For UTF-8 that number is 4, not 3. A quick look at the source for the
default UTF-8 encoder confirms that there are cases where it will output
4 bytes for a single input character.

Mark


> 
> 
> changeset:   3116:b44704ce8a08
> user:sherman
> date:2010-11-19 12:58 -0800
> 6957230: CharsetEncoder.maxBytesPerChar() reports 4 for UTF-8; should be 3
> Summary: changged utf-8's CharsetEncoder.maxBytesPerChar to 3
> Reviewed-by: alanb
> 
> 
> On Mon, Sep 22, 2014 at 1:14 PM, Ivan Gerasimov 
> wrote:
> 
>> Hello!
>>
>> The UTF-8 encoding allows characters that are 4 bytes long.
>> However, CharsetEncoder.maxBytesPerChar() currently returns 3.0, which is
>> not always enough.
>>
>> Would you please review the simple fix for this issue?
>>
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8058875
>> WEBREV: http://cr.openjdk.java.net/~igerasim/8058875/0/webrev/
>>
>> Sincerely yours,
>> Ivan
>>

Re: RFR [8058875]: CharsetEncoder.maxBytesPerChar() should return 4 for UTF-8

[Xueming Shen]

On 09/22/2014 01:14 PM, Ivan Gerasimov wrote:

Hello!

The UTF-8 encoding allows characters that are 4 bytes long.
However, CharsetEncoder.maxBytesPerChar() currently returns 3.0, which is not 
always enough.

Would you please review the simple fix for this issue?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8058875
WEBREV: http://cr.openjdk.java.net/~igerasim/8058875/0/webrev/

Sincerely yours,
Ivan


The "character" in the nio Charset and CharDe/Encoder is specified as 
"sixteen-bit Unicode
code unit", so it is reasonable to interpret the "character" in the "maximum 
number of bytes
that will be produced for each character of input" to be the Java "char" as 
well. In case of
UTF8, each 4-byte form supplementary character is always coded into 2 surrogate 
chars,
it's "2 byte per char". Do we have a real escalation that complains about this?

-Sherman

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Mike Duigou]

Looks fine.

I think it is always important note when a change may result in different 
results for some inputs. I will reiterate for the record what's mentioned in 
the bug:

> However, one caveat is that this may affect the results of some calculations.
> For example, some range of numbers that used to overflow to infinity by
> performing the multiplication by 180, will now not overflow and will return a
> valid result.

This also applies to very small quantities in toRadians where dividing by 180 
may have previously resulted in a zero.

Cheers,

Mike

On Sep 22 2014, at 14:10 , Brian Burkhalter  wrote:

> Hello,
> 
> Another relatively small numerics fix:
> 
> Issue:https://bugs.openjdk.java.net/browse/JDK-4477961
> Webrev:   http://cr.openjdk.java.net/~bpb/4477961/webrev.00/
> 
> Summary: Change constructs like “a * B / C” and “u / V * W” to “x * (Y / Z)” 
> where lower case denotes a variable and upper case a constant. This forces 
> the constant value (Y / Z) to be evaluated only once per VM instance, and 
> replaces division of the variable with multiplication. The resulting 
> performance improvement is more than 300% as measure by JMH on a 
> MacBookPro11,1 dual core i7 running Mac OS 10.9.5.
> 
> Thanks,
> 
> Brian

Re: RFR [8058875]: CharsetEncoder.maxBytesPerChar() should return 4 for UTF-8

[Martin Buchholz]

I think you are mistaken. It's maxBytesPerChar, not maxBytesPerCodepoint!


changeset:   3116:b44704ce8a08
user:sherman
date:2010-11-19 12:58 -0800
6957230: CharsetEncoder.maxBytesPerChar() reports 4 for UTF-8; should be 3
Summary: changged utf-8's CharsetEncoder.maxBytesPerChar to 3
Reviewed-by: alanb


On Mon, Sep 22, 2014 at 1:14 PM, Ivan Gerasimov 
wrote:

> Hello!
>
> The UTF-8 encoding allows characters that are 4 bytes long.
> However, CharsetEncoder.maxBytesPerChar() currently returns 3.0, which is
> not always enough.
>
> Would you please review the simple fix for this issue?
>
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8058875
> WEBREV: http://cr.openjdk.java.net/~igerasim/8058875/0/webrev/
>
> Sincerely yours,
> Ivan
>

JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

Hello,

Another relatively small numerics fix:

Issue:  https://bugs.openjdk.java.net/browse/JDK-4477961
Webrev: http://cr.openjdk.java.net/~bpb/4477961/webrev.00/

Summary: Change constructs like “a * B / C” and “u / V * W” to “x * (Y / Z)” 
where lower case denotes a variable and upper case a constant. This forces the 
constant value (Y / Z) to be evaluated only once per VM instance, and replaces 
division of the variable with multiplication. The resulting performance 
improvement is more than 300% as measure by JMH on a MacBookPro11,1 dual core 
i7 running Mac OS 10.9.5.

Thanks,

Brian

Re: RFR: 8058887: (fmt) Improve java/util/Formatter test coverage of group separators and width

[Xueming Shen]

On 09/22/2014 02:00 PM, Xueming Shen wrote:

It was on purpose to keep those blank lines when I wrote the Spp to replace the 
original
script. it serves the purpose of preserving  the line number the generated 
source code
(identical to the ln# of the same code line in template file). This is 
convenient/import


meant to say "convenient/important" :-)



when debugging those generated nio source files with a specific line number, 
from
exception stack trace, for example.

-sherman

On 09/22/2014 12:02 PM, Claes Redestad wrote:

Hi,

previous attempt was considered too trivial a fix, so how about actually 
improving the test coverage, fixing the link issue in genBasic and upgrade 
Spp.java to not add thousands of empty lines while we're at it? Rejoice!

webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.03/

Most relevant files to review are:

http://cr.openjdk.java.net/~redestad/8058887/webrev.03/test/java/util/Formatter/Basic-X.java.template.udiff.html
http://cr.openjdk.java.net/~redestad/8058887/webrev.03/make/src/classes/build/tools/spp/Spp.java.udiff.html
http://cr.openjdk.java.net/~redestad/8058887/webrev.03/test/java/util/Formatter/genBasic.sh.udiff.html

... while the others are generated from the update Spp.java tool using the 
fixed genBasic.sh.

/Claes

On 09/22/2014 04:35 PM, Claes Redestad wrote:

Hi,

can I have a review and sponsor for this trivial fix to make the jtreg test 
generator script jdk/test/java/util/Formatter/genBasic.sh work in the new build 
system?

bug: https://bugs.openjdk.java.net/browse/JDK-8058887
webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.01/

/Claes

Re: RFR: 8058887: (fmt) Improve java/util/Formatter test coverage of group separators and width

[Xueming Shen]

It was on purpose to keep those blank lines when I wrote the Spp to replace the 
original
script. it serves the purpose of preserving  the line number the generated 
source code
(identical to the ln# of the same code line in template file). This is 
convenient/import
when debugging those generated nio source files with a specific line number, 
from
exception stack trace, for example.

-sherman

On 09/22/2014 12:02 PM, Claes Redestad wrote:

Hi,

previous attempt was considered too trivial a fix, so how about actually 
improving the test coverage, fixing the link issue in genBasic and upgrade 
Spp.java to not add thousands of empty lines while we're at it? Rejoice!

webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.03/

Most relevant files to review are:

http://cr.openjdk.java.net/~redestad/8058887/webrev.03/test/java/util/Formatter/Basic-X.java.template.udiff.html
http://cr.openjdk.java.net/~redestad/8058887/webrev.03/make/src/classes/build/tools/spp/Spp.java.udiff.html
http://cr.openjdk.java.net/~redestad/8058887/webrev.03/test/java/util/Formatter/genBasic.sh.udiff.html

... while the others are generated from the update Spp.java tool using the 
fixed genBasic.sh.

/Claes

On 09/22/2014 04:35 PM, Claes Redestad wrote:

Hi,

can I have a review and sponsor for this trivial fix to make the jtreg test 
generator script jdk/test/java/util/Formatter/genBasic.sh work in the new build 
system?

bug: https://bugs.openjdk.java.net/browse/JDK-8058887
webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.01/

/Claes

Re: RFR (XXS): 8058887: TEST_BUG: java/util/Formatter/genBasic.sh points to old location of Spp.java

[Xueming Shen]

On 09/22/2014 07:35 AM, Claes Redestad wrote:

Hi,

can I have a review and sponsor for this trivial fix to make the jtreg test 
generator script jdk/test/java/util/Formatter/genBasic.sh work in the new build 
system?

bug: https://bugs.openjdk.java.net/browse/JDK-8058887
webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.01/

/Claes



looks good.

RFR [8058875]: CharsetEncoder.maxBytesPerChar() should return 4 for UTF-8

[Ivan Gerasimov]

Hello!

The UTF-8 encoding allows characters that are 4 bytes long.
However, CharsetEncoder.maxBytesPerChar() currently returns 3.0, which 
is not always enough.


Would you please review the simple fix for this issue?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8058875
WEBREV: http://cr.openjdk.java.net/~igerasim/8058875/0/webrev/

Sincerely yours,
Ivan

Re: RFR [9]: 8050142: Optimize java.util.Formatter

[Claes Redestad]

Hi,

Sherman pointed out that there was a path that could actually take a 
minor performance hit from this patch,
which would be unacceptable. This version takes the minimal approach to 
addressing this by adding back a
method operating on a char[], simplified for the specific usage case 
(the exponent part of a %g double formatting):


http://cr.openjdk.java.net/~redestad/8050142/webrev.07/

This latest patch passes using the extended test coverage of 
java.util.Formatter I've proposed in 8058887, see

http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.html

/Claes

On 09/16/2014 03:15 PM, Claes Redestad wrote:

Brent, Marcus,

thank you for reviewing!

JCK 9 tests for api/java_util api/java_lang either pass or fail with 
or without this patch on my machine.


Also reran relevant jtreg tests (jdk/test/java/util/Formatter 
jdk/test/java/util/Formattable jdk/test/java/lang/String 
jdk/test/java/lang/System)


Ok to fix nits offline without posting new webrev?

/Claes

On 09/16/2014 11:34 AM, Marcus Lagergren wrote:
I am reviewer and they look good to me too. ~30% better performance 
for random indata- Do you pass the JCK? If so you have my blessing.


/M

On 16 Sep 2014, at 01:52, Brent Christian 
 wrote:



Hi, Claes

I've looked over the latest changes, and they look good to me.  I 
can sponsor this.  Note that we need approval from a Reviewer.


One small nitpick from me:
2914:
If the text is left-justified, then aren't we padding on the right?  
I would call this "padRight".


Thanks,
-Brent

On 7/14/14 3:41 PM, Claes Redestad wrote:
Sorry about that; maybe I should've renamed the field c to conv 
instead,
but I think I need to be conservative now and will revert the name 
changes.


New webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.3

Changing usage of given locale or any semantic change is really
out-of-scope here. There seems to be a few ancient bugs hanging around
which maybe someone should take a look at, e.g.,
https://bugs.openjdk.java.net/browse/JDK-5063466

/Claes

On 2014-07-14 20:05, Jason Mehrens wrote:

Claes,


Maybe change (or not):


-throw new UnknownFormatConversionException(String.valueOf(c));

+throw new UnknownFormatConversionException(String.valueOf(conv));



I haven't examined it too deeply but it seems odd that some of those
print methods don't use the given locale when converting case.  That
is probably not a change for this issue.


Jason






Date: Mon, 14 Jul 2014 17:40:41 +0200
From: claes.redes...@oracle.com
To: core-libs-dev@openjdk.java.net
Subject: Re: RFR [9]: 8050142: Optimize java.util.Formatter

Hi,

final(?) webrev: 
http://cr.openjdk.java.net/~redestad/8050142/webrev.2


Thanks in advance for reviews. I also need a volunteer to sponsor
this. :-)

/Claes

On 07/14/2014 04:21 PM, Claes Redestad wrote:

Yes, might be worth addressing just for correctness/readability.

/Claes

On 07/14/2014 03:02 PM, Ivan Gerasimov wrote:

A very minor one:
2704 if (Character.isUpperCase(conv))
2705 f.add(Flags.UPPERCASE);
2706 c = Character.toLowerCase(conv);

maybe

2704 if (Character.isUpperCase(conv)) {
2705 f.add(Flags.UPPERCASE);
2706 c = Character.toLowerCase(conv);
}

Sincerely yours,
Ivan


On 14.07.2014 16:23, Claes Redestad wrote:

Hi again,

updated webrev: 
http://cr.openjdk.java.net/~redestad/8050142/webrev.1


changes:
- specify capacity on line 2931 as suggested by Andrej Golovnin
- exp.append("0") -> exp.append('0') on line 3781
- merged append+justify into appendJustified as suggested by 
Peter

Levart
- replaced the reoccuring pattern of appending a number of zeros
into a call to trailingZeros

performance difference seemingly at noise levels in micros, but
bonus to readability and Formatter*.class-files are now a 
total of

246 bytes smaller

/Claes

On 2014-07-14 13:29, Claes Redestad wrote:

Hi Peter,

On 2014-07-14 13:25, Peter Levart wrote:

On 07/14/2014 12:07 PM, Claes Redestad wrote:

Hi,

please review this patch which optimizes away some allocations
from java.util.Formatter and achieve 1.1-1.3x speedups of 
micros

targetting String.format. See bug for more details.

webrev: http://cr.openjdk.java.net/~redestad/8050142/webrev.0
bug: https://bugs.openjdk.java.net/browse/JDK-8050142

Testing: JPRT, jtreg (java/lang/String, java/util/Formatter),
SPECjbb2013 and microbenchmarks

Thanks!

/Claes

Hi Claes,

Since justify() result is always appended to the resulting
Appendable, you could merge the functionality and eliminate
constructing intermediary StringBuilder altogether:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Formatter/webrev.01/ 




Looks good, especially eliminating the need for two different
append methods. I'll update based on this and other suggestions.

/Claes

Regards, Peter

RFR: 8058887: (fmt) Improve java/util/Formatter test coverage of group separators and width

[Claes Redestad]

Hi,

previous attempt was considered too trivial a fix, so how about actually 
improving the test coverage, fixing the link issue in genBasic and 
upgrade Spp.java to not add thousands of empty lines while we're at it? 
Rejoice!


webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.03/

Most relevant files to review are:

http://cr.openjdk.java.net/~redestad/8058887/webrev.03/test/java/util/Formatter/Basic-X.java.template.udiff.html
http://cr.openjdk.java.net/~redestad/8058887/webrev.03/make/src/classes/build/tools/spp/Spp.java.udiff.html
http://cr.openjdk.java.net/~redestad/8058887/webrev.03/test/java/util/Formatter/genBasic.sh.udiff.html

... while the others are generated from the update Spp.java tool using 
the fixed genBasic.sh.


/Claes

On 09/22/2014 04:35 PM, Claes Redestad wrote:

Hi,

can I have a review and sponsor for this trivial fix to make the jtreg 
test generator script jdk/test/java/util/Formatter/genBasic.sh work in 
the new build system?


bug: https://bugs.openjdk.java.net/browse/JDK-8058887
webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.01/

/Claes

Re: JDK 9 RFR of 8043740: Doubles with large exponents overflow to Infinity incorrectly

[Joe Darcy]

Hi Brian,

Okay; looks good to go back.

Thanks,

-Joe

On 9/22/2014 8:53 AM, Brian Burkhalter wrote:

Hi Joe,

Yes, and then some. There are two cases which fail before applying the 
patch and neither fails thereafter. One of these cases matches the 
test in the original patch. The remaining cases pass both before and 
after but I added them as edges cases anyway.


Thanks,

Brian

On Sep 21, 2014, at 9:52 PM, Joe Darcy > wrote:


Do the additional cases in the regression tests full cover the 
proposed revision of the code changes?

Re: JDK 9 RFR of 8043740: Doubles with large exponents overflow to Infinity incorrectly

[Brian Burkhalter]

Hi Joe,

Yes, and then some. There are two cases which fail before applying the patch 
and neither fails thereafter. One of these cases matches the test in the 
original patch. The remaining cases pass both before and after but I added them 
as edges cases anyway.

Thanks,

Brian

On Sep 21, 2014, at 9:52 PM, Joe Darcy  wrote:

> Do the additional cases in the regression tests full cover the proposed 
> revision of the code changes?

Re: Urgent [9] RFR (S) : JDK-8039915 NumberFormat.format() does not consider required no. of fraction digits properly

[Brian Burkhalter]

Hi Olivier,

On Sep 22, 2014, at 7:56 AM, olivier.lagn...@oracle.com wrote:

>> and 2) use braces around all the statements contained in if/else blocks (see 
>> below). Comment #2 is nit-picky.
> I tried to keep the same flavour of writing as in HALF_DOWN and HALF_EVEN 
> cases, i.e. don't use brace for
> terminal/leaf return true/false statements. This is not the standard however, 
> at least in this file.
> Will use braces in all case (i.e. the 3 of HALF_UP,  HALF_DOWN and HALF_EVEN).

I did not look at the other case. If your formatting matches the rest of the 
file then I think it is OK to leave it as-is.

>> Lastly and this is not really part of your changeset, but I found it curious 
>> that the test prints the details of all cases that succeed as opposed to 
>> those that fail. I would think that either all results or the failures alone 
>> ought to be printed instead of successes only. See for example the partial 
>> diff below the DigitList diff.
> Since these are most often corner and tricky test cases I think it 
> interesting to have the details of each result,
> including infos of both why returned result is correct or wrong.
> That can help the reader to understand all these tricky cases.
> The bad side of it being that it prints a lot of text, with failure cases 
> (hoepfully few) lost in the middle of it,
> thus making failures possibly not immediate to detect. 
> 
> Here is an example of what is printed in case of failure:
> "
> 
> ***Error formatting double value from string : 0.6868d
> NumberFormat pattern is  : #,##0.###
> Maximum number of fractional digits : 3
> Fractional rounding digit : 4
> Position of value relative to tie : above
> Rounding Mode : HALF_UP
> BigDecimal output : 0.68676607158436745521612465381622314453125
> FloatingDecimal output : 0.6868
> Error. Formatted result different from expected.
> Expected output is : "0.687"
> Formated output is : "0.686"
> 

I missed that output: I was looking for the word “failure.”

> There is also a reminder of the number of errors at the end of the report:
> "
> ==> 4 tests failed
> 
> Test failed with 4 formating error(s).
> "
> 
> May be providing a reminder (value + rounding-mode + rounding position)
> of the failure cases at the end of the would be better ?
> Like:
> "
> Test failed with 4 formating error(s) for following cases :
> - 0.3126d, HALF_UP rounding, 3 fractional digits
> - 0.6868d, HALF_UP rounding, 3 fractional digits
> - 1.798876d, HALF_UP rounding, 5 fractional digits
> - 1.796889d, HALF_UP rounding, 5 fractional digits
> "
> 
> Would doing so be ok ?

If the test is already printing out the information you showed above (“Error 
formatting …”) then I think it is enough but the verbiage should perhaps match 
the reminder, e.g., “Failure: Error formatting double …”

Thanks,

Brian

Re: Urgent [9] RFR (S) : JDK-8039915 NumberFormat.format() does not consider required no. of fraction digits properly

[olivier.lagn...@oracle.com]

Hi Brian,

Thanks a lot for your thorough review of the fix !

Please see comments inlined.

On 20/09/2014 00:50, Brian Burkhalter wrote:

Hello Olivier,

By inspection I think that the fix and the test update look good. I 
verified that the test hits all five branches contained in the else-if 
block of DigitList beginning at line 527. I also verified that the 
current code base fails four test cases when run against the updated 
test. The welcome testing performed by William Price is good 
corroboration as well.

Thanks for checking all these. with details of


My only suggestions are trivial: 1) enhance the method javadoc of 
shouldRoundUp(), e.g., there are no @param tags for the boolean 
parameters,

Did not notice that. Will add those.
and 2) use braces around all the statements contained in if/else 
blocks (see below). Comment #2 is nit-picky.
I tried to keep the same flavour of writing as in HALF_DOWN and 
HALF_EVEN cases, i.e. don't use brace for
terminal/leaf return true/false statements. This is not the standard 
however, at least in this file.
Will use braces in all case (i.e. the 3 of HALF_UP,  HALF_DOWN and 
HALF_EVEN).




Lastly and this is not really part of your changeset, but I found it 
curious that the test prints the details of all cases that succeed as 
opposed to those that fail. I would think that either all results or 
the failures alone ought to be printed instead of successes only. See 
for example the partial diff below the DigitList diff.
Since these are most often corner and tricky test cases I think it 
interesting to have the details of each result,

including infos of both why returned result is correct or wrong.
That can help the reader to understand all these tricky cases.
The bad side of it being that it prints a lot of text, with failure 
cases (hoepfully few) lost in the middle of it,

thus making failures possibly not immediate to detect.

Here is an example of what is printed in case of failure:
"

***Error formatting double value from string : 0.6868d
NumberFormat pattern is  : #,##0.###
Maximum number of fractional digits : 3
Fractional rounding digit : 4
Position of value relative to tie : above
Rounding Mode : HALF_UP
BigDecimal output : 0.68676607158436745521612465381622314453125
FloatingDecimal output : 0.6868
Error. Formatted result different from expected.
Expected output is : "0.687"
Formated output is : "0.686"

"

There is also a reminder of the number of errors at the end of the report:
"
==> 4 tests failed

Test failed with 4 formating error(s).
"

May be providing a reminder (value + rounding-mode + rounding position)
of the failure cases at the end of the would be better ?
Like:
"
Test failed with 4 formating error(s) for following cases :
- 0.3126d, HALF_UP rounding, 3 fractional digits
- 0.6868d, HALF_UP rounding, 3 fractional digits
- 1.798876d, HALF_UP rounding, 5 fractional digits
- 1.796889d, HALF_UP rounding, 5 fractional digits
"

Would doing so be ok ?

Best Regards,
Olivier.



Thanks,

Brian

--- a/src/java.base/share/classes/java/text/DigitList.java
+++ b/src/java.base/share/classes/java/text/DigitList.java
@@ -526,24 +526,25 @@
 }
 else if (digits[maximumDigits] == '5') {
 // Digit at rounding position is a '5'. Tie cases.
-if (maximumDigits != (count - 1))
+if (maximumDigits != (count - 1)) {
 // Remaining digits. Above tie  => must round-up
 return true;
-else {
+} else {
 // Digit at rounding position is the last one !
-if (allDecimalDigits)
+if (allDecimalDigits) {
 // Exact binary representation. On the tie.
 // Strictly follow HALF_UP rule ==> round-up
 return true;
-else {
-if (alreadyRounded)
+} else {
+if (alreadyRounded) {
 // Digit sequence rounded-up. Was 
below tie.

 // Don't round-up twice !
 return false;
-else
+} else {
 // Digit sequence truncated. Was 
above tie

 // HALF_UP rule ==>  must round-up !
 return true;
+}
 }
 }
 }

test/java/text/Format/DecimalFormat/TieRoundingTest.java

 errorCounter++;
 allPassed = false;
+System.out.println("\nFailure for double value : " + 
doubleToTest + " :");

 } else {
 testCo

RFR (XXS): 8058887: TEST_BUG: java/util/Formatter/genBasic.sh points to old location of Spp.java

[Claes Redestad]

Hi,

can I have a review and sponsor for this trivial fix to make the jtreg 
test generator script jdk/test/java/util/Formatter/genBasic.sh work in 
the new build system?


bug: https://bugs.openjdk.java.net/browse/JDK-8058887
webrev: http://cr.openjdk.java.net/~redestad/8058887/webrev.01/

/Claes

Re: RFR (XS) CR 8058643: (str) Re-examine hashCode implementation

[Aleksey Shipilev]

On 09/22/2014 06:13 PM, Aleksey Shipilev wrote:
> On 09/22/2014 06:06 PM, Aleksey Shipilev wrote:
>> Hi again,
>>
>> On 09/17/2014 06:28 PM, Aleksey Shipilev wrote:
>>> Can I have a review and a sponsorship for this tiny readability cleanup
>>> in String.hashCode()?
>>>  http://cr.openjdk.java.net/~shade/8058643/webrev.01/
>>>  https://bugs.openjdk.java.net/browse/JDK-8058643
>>
>> I think we have enough reviews? Here is a changeset:
>>   http://cr.openjdk.java.net/~shade/8058643/8058643.changeset
>>
>> Please sponsor!
> 
> Wait. Claes spotted an inconsistency:
> 
> Should be:
>  for (char v : value) {
> 
> Not:
>  for (int v : value) {

Fixed:
 http://cr.openjdk.java.net/~shade/8058643/webrev.02/
 http://cr.openjdk.java.net/~shade/8058643/8058643.changeset

Since there is no performance impact for this touchup (char -> int
promotion is happening anyway), and no functionality changes (char ->
int promotion zero-extends), I haven't developed any new tests, or
re-spinned any existing ones.

Thanks,
-Aleksey.

Re: RFR (XS) CR 8058643: (str) Re-examine hashCode implementation

[Aleksey Shipilev]

On 09/22/2014 06:06 PM, Aleksey Shipilev wrote:
> Hi again,
> 
> On 09/17/2014 06:28 PM, Aleksey Shipilev wrote:
>> Can I have a review and a sponsorship for this tiny readability cleanup
>> in String.hashCode()?
>>  http://cr.openjdk.java.net/~shade/8058643/webrev.01/
>>  https://bugs.openjdk.java.net/browse/JDK-8058643
> 
> I think we have enough reviews? Here is a changeset:
>   http://cr.openjdk.java.net/~shade/8058643/8058643.changeset
> 
> Please sponsor!

Wait. Claes spotted an inconsistency:

Should be:
 for (char v : value) {

Not:
 for (int v : value) {

Let me also write a unit test for this :)

-Aleksey.

Re: RFR (XS) CR 8058643: (str) Re-examine hashCode implementation

[Aleksey Shipilev]

Hi again,

On 09/17/2014 06:28 PM, Aleksey Shipilev wrote:
> Can I have a review and a sponsorship for this tiny readability cleanup
> in String.hashCode()?
>  http://cr.openjdk.java.net/~shade/8058643/webrev.01/
>  https://bugs.openjdk.java.net/browse/JDK-8058643

I think we have enough reviews? Here is a changeset:
  http://cr.openjdk.java.net/~shade/8058643/8058643.changeset

Please sponsor!

Thanks,
-Aleksey.

Re: Urgent [9] RFR (S) : JDK-8039915 NumberFormat.format() does not consider required no. of fraction digits properly

[olivier.lagn...@oracle.com]

Hi William,

Thanks a lot for your time and checks !

On 19/09/2014 22:02, William Price wrote:

Hi Oliver,

First, sorry about mistyping your name, Olivier!


I copied your patch into my shim locally and ran my test cases and
still get a couple failures (see output below).  Your patch and my version
differ in the way and order in which we interpret allDecimalDigits and
alreadyRounded.

And I should have been more diligent. On second look these are all "very close to a 
tie"
as described in JDK-7131459, so I now believe my test suite is incorrect.  It 
matched the
JDK 6-7 behavior but did not account for the JDK8 intended changes for these 
edge cases.
Yes these are voluntarily all tricky cases with dtoa() returning an 
exact tie

due to IEEE-754 precision limit, thus following nearest to even policy and
either rounding up or truncating right at the rounding digit position.

allDecimalDigits indicates whether dtoa() was able to provide all the 
exact digits,
with result still inside IEEE-754 precision. alreadyRounded indicates 
whether the result

was truncated or rounded up.

I think allDecimalDigits must be checked first, followed by 
alreadyRounded, since
if you get an exact binary representation within IEEE precision, dtoa 
won't do

any rounding by principle.
I believe the latest (last Friday) version of your patch on github is ok.

Thanks again William for following this bug,
and for the feedback and checking of the patch for this review  !

Best Regards,
Olivier.



JDK-7131459 test case:
0.15 is actually: 
0.1499944488848768742172978818416595458984375
 HALF_UP  --> 0.1OK
0.35 is actually: 
0.34997779553950749686919152736663818359375
 HALF_UP  --> 0.3OK
0.85 is actually: 
0.84997779553950749686919152736663818359375
 HALF_UP  --> 0.8OK
0.95 is actually: 
0.9499555910790149937383830547332763671875
 HALF_UP  --> 0.9OK

Above tests used Java 1.8.0_20 (Oracle Corporation)
installed at C:\JAVA\latest8\jre

Agent installed: yes
Patch applied  : yes

Overall result : FIXED
  

Re: Lower overhead String encoding/decoding

[Ulf Zibis]

Compare with https://bugs.openjdk.java.net/browse/JDK-6695386
Maybe that would help a little.

-Ulf


Am 22.09.2014 um 13:25 schrieb Richard Warburton:

Hi all,

A long-standing issue with Strings in Java is the ease and performance of
creating a String from a ByteBuffer. People who are using nio to bring in
data off the network will be receiving that data in the form of bytebuffers
and converting it to some form of String. For example restful systems
receiving XML or Json messages.

The current workaround is to create a byte[] from the ByteBuffer - a
copying action for any direct bytebuffer - and then pass that to the
String. I'd like to propose that we add an additional constructor to the
String class that takes a ByteBuffer as an argument, and directly create
the char[] value inside the String from the ByteBuffer.

Similarly if you have a String that you want to encode onto the wire then
you need to call String.getBytes(), then write your byte[] into a
ByteBuffer or send it over the network. This ends up allocating a byte[] to
do the copy and also trimming the byte[] back down again, usually
allocating another byte[]. To address this problem I've added a couple of
getBytes() overloads that take byte[] and ByteBuffer arguments and write
directly to those buffers.

I've put together a patch that implements this to demonstrate the overall
direction.

http://cr.openjdk.java.net/~rwarburton/string-patch-webrev-5/

I'm happy to take any feedback on direction or the patch itself or the
overall idea/approach. I think there are a number of similar API situations
in other places as well, for example StringBuffer/StringBuilder instances
which could have similar appends directly from ByteBuffer instances instead
of byte[] instances.

I'll also be at Javaone next week, so if you want to talk about this, just
let me know.

regards,

   Richard Warburton

   http://insightfullogic.com
   @RichardWarburto 

PS: I appreciate that since I'm adding a method to the public API which
consequently requires standardisation but I think that this could get
incorporated into the Java 9 umbrella JSR.

Re: Lower overhead String encoding/decoding

[Alan Bateman]

On 22/09/2014 12:25, Richard Warburton wrote:

:

I've put together a patch that implements this to demonstrate the overall
direction.

http://cr.openjdk.java.net/~rwarburton/string-patch-webrev-5/

I'm happy to take any feedback on direction or the patch itself or the
overall idea/approach. I think there are a number of similar API situations
in other places as well, for example StringBuffer/StringBuilder instances
which could have similar appends directly from ByteBuffer instances instead
of byte[] instances.

The direction seems reasonable but I wonder about the offset/length (and 
destOffet) parameters as this isn't consistent with how ByteBuffers were 
originally intended to be used. That is, when you read the bytes from 
the wire into a ByteBuffer and flip it then the position and limit will 
delimit the bytes to be decoded.


If the constructor is changed to String(ByteBuffer in, Charset cs) and 
decodes the remaining bytes in the buffer to a String using the 
specified Charset then I think would be more consistent. Also I think 
this would give you a solution to the underflow case.


Similarly if getBytes is replaced with with a getBytes or 
encode(ByteBuffer, Charset cs) then then it would encode as many 
characters as possible into the output buffer and I think would be more 
consistent and also help with overflow case.


-Alan.

Lower overhead String encoding/decoding

[Richard Warburton]

Hi all,

A long-standing issue with Strings in Java is the ease and performance of
creating a String from a ByteBuffer. People who are using nio to bring in
data off the network will be receiving that data in the form of bytebuffers
and converting it to some form of String. For example restful systems
receiving XML or Json messages.

The current workaround is to create a byte[] from the ByteBuffer - a
copying action for any direct bytebuffer - and then pass that to the
String. I'd like to propose that we add an additional constructor to the
String class that takes a ByteBuffer as an argument, and directly create
the char[] value inside the String from the ByteBuffer.

Similarly if you have a String that you want to encode onto the wire then
you need to call String.getBytes(), then write your byte[] into a
ByteBuffer or send it over the network. This ends up allocating a byte[] to
do the copy and also trimming the byte[] back down again, usually
allocating another byte[]. To address this problem I've added a couple of
getBytes() overloads that take byte[] and ByteBuffer arguments and write
directly to those buffers.

I've put together a patch that implements this to demonstrate the overall
direction.

http://cr.openjdk.java.net/~rwarburton/string-patch-webrev-5/

I'm happy to take any feedback on direction or the patch itself or the
overall idea/approach. I think there are a number of similar API situations
in other places as well, for example StringBuffer/StringBuilder instances
which could have similar appends directly from ByteBuffer instances instead
of byte[] instances.

I'll also be at Javaone next week, so if you want to talk about this, just
let me know.

regards,

  Richard Warburton

  http://insightfullogic.com
  @RichardWarburto 

PS: I appreciate that since I'm adding a method to the public API which
consequently requires standardisation but I think that this could get
incorporated into the Java 9 umbrella JSR.

Re: extcheck

[Alan Bateman]

On 11/09/2014 17:23, Pavel Rappo wrote:

:

We don't think it is widely used. And will become even less useful with 
upcoming modularization. Are there any good reasons to keep this thing in the 
JDK?

Searching for usages on stackoverflow and other sites doesn't come up 
with much, it would be surprising to find significant usage nowadays. So 
I can't think of any reason to keep it around.


-Alan.

Re: RFR JDK-8044627: Update JNDI to work with modules

[Alan Bateman]

On 16/09/2014 12:12, Pavel Rappo wrote:

Hi everyone,

Could you please review my change for JDK-8044627?

Pavel - are you planning to send an updated webrev based on the 
discussion so far?


The other thing that I meant to ask is whether this change will add 
service configuration files for the RMI, DNS and CosNaming 
implementations, maybe this is going to be another patch?


-Alan