RE: Oracle Java 8u161 regression in XML Schema Factory

[Langer, Christoph]

Thanks, Joe.

> -Original Message-
> From: Joe Wang [mailto:huizhe.w...@oracle.com]
> Sent: Donnerstag, 1. März 2018 20:11
> To: Langer, Christoph 
> Cc: OpenJDK Dev list 
> Subject: Re: Oracle Java 8u161 regression in XML Schema Factory
> 
> Hi Christoph and all,
> 
> Just wanted to let you know that we're in progress to update the release
> notes for 6u181/7u171/8u161 with a doc for this change.
> 
> Thanks,
> Joe
> 
> On 2/22/2018 12:47 AM, Langer, Christoph wrote:
> > Hi Joe,
> >
> > thanks for the clarification. It would be good to have a place of
> documentation where one could refer customers to.
> >
> > Thanks
> > Christoph
> >
> >> -Original Message-
> >> From: Joe Wang [mailto:huizhe.w...@oracle.com]
> >> Sent: Mittwoch, 21. Februar 2018 21:50
> >> To: Langer, Christoph 
> >> Cc: Bernd ; OpenJDK Dev list  >> d...@openjdk.java.net>
> >> Subject: Re: Oracle Java 8u161 regression in XML Schema Factory
> >>
> >>
> >>> @Joe: Is there some documentation for this change in default behavior
> >> that came with JDK8u161? I assume it is for higher security and cannot be
> >> reverted (e.g. by setting the feature default for
> >> Djdk.xml.overrideDefaultParser to true).
> >>
> >> It is indeed. It was a customer's request. Customers' complaints were
> >> that a factory created through the official API could in many cases,
> >> unknowingly to the customers, load 3rd party parsers that didn't
> >> necessarily implement the security features added since JDK7u40 and 8.
> >> For customers, this behavior was a security concern. It was particularly
> >> inconvenient to users who might have some 3rd party libraries that just
> >> happen to be in their environment.
> >>
> >> This change was not mentioned in the release notes. I'll check whether
> >> we could find a right place for a note of this change. The 8u161 release
> >> notes would have been a good place to do so.
> >>
> >> Best,
> >> Joe
> >>
> >>> Best regards
> >>> Christoph
> >>>
>  -Original Message-
>  From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net]
> On
>  Behalf Of Bernd
>  Sent: Dienstag, 13. Februar 2018 22:55
>  To: OpenJDK Dev list
>  Subject: Re: Oracle Java 8u161 regression in XML Schema Factory
> 
>  Hello,
> 
> 
>  2018-01-25 17:41 GMT+01:00 Seán Coffey:
> 
> > Classes nearer to those below were touched via JDK-8186080:
> Transform
>  XML
> > interfaces
> > http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/cb84156d54b2
> > http://hg.openjdk.java.net/jdk8u/jdk8u/jaxp/rev/08a44c164993
> >
> > This may be connected with some tools trying to redefine the classes
> > perhaps. Needs more investigating. Perhaps the XMLSchemaLoader
>  changes are
> > a factor ?
> >
>  I have ben able to extract a minimal reproducer. It is not related to
>  XMLUnit, only to powermock. If it instruments com.sun but not
> javax.xml
>  (and other combinations) then it fails.
> 
>  For details see the readme in this maven project:
> 
>  https://github.com/ecki/reproduce-schemaex
> 
>  I also found a way to make it work with both versions, so its no longer
> an
>  issue for me, but there is definitely some changes (which might also be
>  triggered in AppServers or OSGi containers with partially reconfigured
>  implementations. Not sure if you want to investigate deeper).
> 
>  Gruss
>  Bernd
> 
> 
>  Here is the stacktrace anyway:
> >> com.sun.org.apache.xerces.internal.impl.dv.DVFactoryException:
> >> Schema
> >> factory class
> >>
> com.sun.org.apache.xerces.internal.impl.dv.xs.SchemaDVFactoryImpl
>  does
> >> not
> >> extend from SchemaDVFactory.
> >>at
> >> com.sun.org.apache.xerces.internal.impl.dv.SchemaDVFactory.
> >> getInstance(SchemaDVFactory.java:75)
> >>at
> >> com.sun.org.apache.xerces.internal.impl.dv.SchemaDVFactory.
> >> getInstance(SchemaDVFactory.java:57)
> >>at
> >> com.sun.org.apache.xerces.internal.impl.xs.XMLSchemaLoader.
> >> reset(XMLSchemaLoader.java:1024)
> >>at
> >> com.sun.org.apache.xerces.internal.impl.xs.XMLSchemaLoader.
> >> loadGrammar(XMLSchemaLoader.java:556)
> >>at
> >> com.sun.org.apache.xerces.internal.impl.xs.XMLSchemaLoader.
> >> loadGrammar(XMLSchemaLoader.java:535)
> >>at
> >> com.sun.org.apache.xerces.internal.jaxp.validation.XMLSchema
> >> Factory.newSchema(XMLSchemaFactory.java:254)
> >>at
> javax.xml.validation.SchemaFactory.newSchema(SchemaFactory.
> >> java:638)
> >>at
> javax.xml.validation.SchemaFactory.newSchema(SchemaFactory.
> >> java:654)
> >>at
> >> 

Re: RFR: Here are some URLClassPath patches

[David Holmes]

On 2/03/2018 2:59 PM, Martin Buchholz wrote:

I have a patch to move those old jdi tests out of the ProblemList jail.
I thought we had an existing bug in hotspot-svc to do this, but now I 
can't find it.


I've created a sub-task of 8198803 for this and assigned it to you.

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


We can just use well-formed URLs.

http://cr.openjdk.java.net/~martin/webrevs/jdk/jdi-tests-malformed-urls/


Looks fine. The proof is in the testing of course :)

Thanks,
David

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

I have a patch to move those old jdi tests out of the ProblemList jail.
I thought we had an existing bug in hotspot-svc to do this, but now I can't
find it.

We can just use well-formed URLs.

http://cr.openjdk.java.net/~martin/webrevs/jdk/jdi-tests-malformed-urls/

Re: RFR: Here are some URLClassPath patches

[David Holmes]

On 2/03/2018 2:21 PM, Martin Buchholz wrote:

Ops.  8198810 is the id of the CSR, not the real bug - I should have
used 8198803.  So I may have broken a jira invariant.  Probably jcheck
should have caught my mistake.  What now?


Now you have to manually update the real bug with the changeset info and 
change its state etc. It will be fixed in build "master" initially then 
when the change hits a promoted build it will need to be changed to the 
actual build number.


And add a comment to the real bug about the incorrect number being used.

Thanks,
David


changeset:   49117:0a93645a57f1  qparent
user:martin
date:2018-03-01 19:01 -0800
8198810: URLClassLoader does not specify behavior when URL array contains
null
Reviewed-by: alanb, darcy, dholmes


On Thu, Mar 1, 2018 at 7:32 PM, Martin Buchholz  wrote:




On Wed, Feb 28, 2018 at 3:58 PM, Martin Buchholz 
wrote:




8198810: URLClassLoader does not specify behavior when URL array contains
null
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassLoader-NPE/
https://bugs.openjdk.java.net/browse/JDK-8198810



  Pushed. Can someone at Oracle help with the release note?

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

Ops.  8198810 is the id of the CSR, not the real bug - I should have
used 8198803.  So I may have broken a jira invariant.  Probably jcheck
should have caught my mistake.  What now?

changeset:   49117:0a93645a57f1  qparent
user:martin
date:2018-03-01 19:01 -0800
8198810: URLClassLoader does not specify behavior when URL array contains
null
Reviewed-by: alanb, darcy, dholmes


On Thu, Mar 1, 2018 at 7:32 PM, Martin Buchholz  wrote:

>
>
> On Wed, Feb 28, 2018 at 3:58 PM, Martin Buchholz 
> wrote:
>
>>
>>
>> 8198810: URLClassLoader does not specify behavior when URL array contains
>> null
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassLoader-NPE/
>> https://bugs.openjdk.java.net/browse/JDK-8198810
>>
>
>  Pushed. Can someone at Oracle help with the release note?
>
>

Re: [11] RFR: 4993841: (str) java.lang.Character should have a toString(int) method

[Martin Buchholz]

The multiple redundant bounds checks bother me, but I don't know how to fix
them without abandoning a bit of modularization.

if (cp >= 0) {
  if (COMPACT_STRINGS && cp < 0x100)  return latin1encode()
  if (cp < MIN_SURROGATE || (cp > MAX_SURROGATE && cp <
MIN_SUPPLEMENTARY_CODEPOINT) return bmpencode()
  if (cp >= MIN_SUPPLEMENTARY_CODEPOINT && cp <= MAX_CODEPOINT) return
suppEncode()
}
throw ...


+static byte[] toBytes(int cp) {
+if (Character.isBmpCodePoint(cp)) {
+return toBytes((char)cp);
+} else {
+byte[] result = new byte[4];
+putChar(result, 0, Character.highSurrogate(cp));
+putChar(result, 1, Character.lowSurrogate(cp));
+return result;
+}
+}

We should continue to assume that supplementary characters are very rare,
even in this context, so the supplementary character code could be moved to
a separate cold method.

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Wed, Feb 28, 2018 at 3:58 PM, Martin Buchholz 
wrote:

>
>
> 8198810: URLClassLoader does not specify behavior when URL array contains
> null
> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassLoader-NPE/
> https://bugs.openjdk.java.net/browse/JDK-8198810
>

 Pushed. Can someone at Oracle help with the release note?

Re: [11] RFR: 4993841: (str) java.lang.Character should have a toString(int) method

[Martin Buchholz]

+ * @param   codepoint   a {@code codePoint}.


does not match the formal parameter.  javadoc -private should give an error
on that.

[11] RFR: 4993841: (str) java.lang.Character should have a toString(int) method

[naoto . sato]

Hi,

Please review the fix to the following issue:

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

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/4993841/webrev.03/

This stems from the recent discussion regarding String.repeat().[1] The 
corresponding CSR has already been approved.


Naoto

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

Re: RFR JDK-8187653: Lock in CoderResult.Cache becomes performance bottleneck

[Xueming Shen]

On 3/1/18, 4:35 PM, David Holmes wrote:

When you replace synchronized code with concurrent data structures you 
introduce race conditions that are precluded in the synchronized code. 
These need to be examined carefully to ensure they are safe. For 
example, whenever you replace a HashMap with a ConcurrentHashMap you 
need to see if put() needs to be replaced by putIfAbsent().




Hi David,

The assumption here is that putIfAbsent() does not help/save anything as 
the value object
would have been created already when it reaches here. And it appears 
there is no need here
to have the check to be atomic, replacing any existing key/value 
pair is fine in this use
scenario.  Was thinking about computIfAbsent(), but concluded it's just 
little overdone (wonder
why we decided to use the WeakReference in this case, it probably is not 
worth it, but keep it as

is for "compatibility" concern).

-Sherman

Re: [JDK 11] RFR 8198821: fix test methods access for test java/text/Normalizer/NormalizerAPITest.java

[Chris Yin]

Thank you, Naoto

Regards,
Chris

> On 2 Mar 2018, at 2:26 AM, Naoto Sato  wrote:
> 
> +1
> 
> Naoto
> 
> On 2/28/18 6:03 PM, Chris Yin wrote:
>> Please review the minor change for test 
>> java/text/Normalizer/NormalizerAPITest.java, thanks
>> Added public access modifier to all “Test_" methods so they can be 
>> recognized as test method correctly by util class
>> bug: https://bugs.openjdk.java.net/browse/JDK-8198821 
>> 
>> webrev: http://cr.openjdk.java.net/~xiaofeya/8198821/webrev.00/ 
>> 
>> Regards,
>> Chris

Re: RFR JDK-8197533 move javax.transaction.xa into its own module

[Paul Sandoz]

> On Mar 1, 2018, at 4:41 PM, Lance Andersen  wrote:
> 
> While running the JDK regression tests, I found a test that needed to be 
> updated, test/langtools/tools/jdeps/modules/DotFileTest.java, due to the 
> update to the java.sql module
> 
> The updated webrev is at 
> http://cr.openjdk.java.net/~lancea/8197533/webrev.03/ 
> 
> 

+1

Paul.

Re: RFR JDK-8197533 move javax.transaction.xa into its own module

[Lance Andersen]

While running the JDK regression tests, I found a test that needed to be 
updated, test/langtools/tools/jdeps/modules/DotFileTest.java, due to the update 
to the java.sql module

The updated webrev is at http://cr.openjdk.java.net/~lancea/8197533/webrev.03/ 


Best
Lance
> On Mar 1, 2018, at 12:37 PM, Paul Sandoz  wrote:
> 
> +1
> 
>> On Mar 1, 2018, at 8:59 AM, Lance Andersen > > wrote:
>>> 
>>> +1, i second Joe’s request to update package-info.java while we are 
>>> opportunistically cleaning this area up.
>> 
>> Per your and Joe’s request, please see 
>> cr.openjdk.java.net/~lancea/8197533/webrev.02 
>>  which has your 
>> suggested update to use package-info.java
>> 
>> Thank you both
>> 
>> Best
>> Lance
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR JDK-8187653: Lock in CoderResult.Cache becomes performance bottleneck

[David Holmes]

Hi Sherman,

On 24/02/2018 11:26 AM, Xueming Shen wrote:

Hi,

Please help review the proposed change to remove the potential performance
bottleneck in CoderResult caused by the "over" synchronization the cache
mechanism.

issue: https://bugs.openjdk.java.net/browse/JDK-8187653
webrev: http://cr.openjdk.java.net/~sherman/8187653/webrev

Notes:
While the original test case (new String/String.getBytes() with UTF8 as 
showed in the
stacktrace) described in the bug report might no longer be reproducible 
in jdk9, as
we have been optimizing lots of String related  char[]/byte[] coding 
path away from
the paths that use CoderResult. But it is still a concern/issue for the 
"general"
CharsetDe/Encoder.de/encode() operation, in which the malformed or 
unmappable

CoderResult object is returned.

As showed in the "[synchronized]" section in bm.scores [1], which is 
from the simple
jmh benchmark test CoderResultBM.java [2], the sores are getting 
significant worse

when the number of concurrent de/encoding threads gets bigger.

It appears the easy fix is to replace the sync mechanism from "method 
synchronized
+ HashMap" to  "ConcurrentHashMap" solves the problem, as showed in the 
same bm
result [1] in [ConcurrentHashMap] section, because most of the accesses 
to the caching
HashMap is read operation. The ConcurrentHahsMap's "almost non-block for 
retrieval

operation" really helps here.


When you replace synchronized code with concurrent data structures you 
introduce race conditions that are precluded in the synchronized code. 
These need to be examined carefully to ensure they are safe. For 
example, whenever you replace a HashMap with a ConcurrentHashMap you 
need to see if put() needs to be replaced by putIfAbsent().


David
-

There is another fact that might help us optimize further here. For most 
of our charsets
in JDK repository (with couple exceptions), the length of malformed and 
unmappable
CoderResult that these charsets might trigger actually is never longer 
than 4. So we might
not need to access the ConcurrentHashMap cache at all in normal use 
scenario. I'm putting
a CoderResult[4] on top the hashmap cache in proposed webrev. It does 
not improve the
performance significantly, but when the thread number gets bigger, a 
10%+ improvement
is observed. So I would assume it might be something worth doing, given 
its cost is really

low.

Thanks,
Sherman


[1] http://cr.openjdk.java.net/~sherman/8187653/bm.scores
[2] http://cr.openjdk.java.net/~sherman/8187653/CoderResultBM.java
[3] http://cr.openjdk.java.net/~sherman/8187653/bm.scores.ms932

Re: 8198888: Reduce string allocation churninInvokerBytecodeGenerator

[mandy chung]

On 3/1/18 12:56 PM, Claes Redestad wrote:


yes, for the startup metrics I'm concerned with then a single element 
implementation is even ever so slightly better:


http://cr.openjdk.java.net/~redestad/819/jdk.01/

Looks good to me.

Mandy

Re: 8198888: Reduce string allocation churninInvokerBytecodeGenerator

[Claes Redestad]

Hi,

On 2018-03-01 20:45, Bernd Eckenfels wrote:

Cool, the lastClass varient then is pretty tempting (and I wont fix the 
multi-slot Version below, it misses a „lastUsedSlot“ field)


yes, for the startup metrics I'm concerned with then a single element 
implementation is even ever so slightly better:


http://cr.openjdk.java.net/~redestad/819/jdk.01/



Unrelated to this patch I feel that unifying internal names and external names 
will be a hard task, so Class#getInternalName() looks temting for a lot of 
similiar usecases mid term.


See Paul's replies elsewhere in this thread about ongoing work on a 
symbolic ref API, which may end up allowing us to express these things 
without having to go via smelly string representations.


/Claes

Re: RFR: JDK-8197594: String#repeat

[Jim Laskey]

Ivan,

The code is now checked in if you want to test.

Cheers,

— Jim


> On Mar 1, 2018, at 4:20 PM, Ivan Gerasimov  wrote:
> 
> Hi Jim!
> 
> I think the copying-loop can be slightly simplified, like this:
> 
> for (; copied < limit - copied; copied <<= 1) {
> System.arraycopy(multiple, 0, multiple, copied, copied);
> }
> 
> (didn't actually run it to check for correctness.)
> 
> With kind regards,
> Ivan
> 
> 
> On 2/28/18 8:31 AM, Jim Laskey wrote:
>> Introduction of a new instance method String::repeat to allow an efficient 
>> and concise approach for generating repeated character sequences as strings.
>> 
>> Performance information in JBS.
>> 
>> Thank you.
>> 
>> Cheers,
>> 
>> — Jim
>> 
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8197594 
>>  
>>  
>> 
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8198296 
>> 
>>   
>> 
>> Webrev: http://cr.openjdk.java.net/~jlaskey/8197594/webrev-02/index.html 
>>  
>>  
>> 
>> JavaDoc: http://cr.openjdk.java.net/~jlaskey/8197594/String.html 
>>  
>>  
>> 
>> 
>> 
>> 
>> 
> 
> -- 
> With kind regards,
> Ivan Gerasimov

Re: JDK-6372077: JarFile.getManifest() should handle manifest attribute names up to 70 bytes

[Xueming Shen]

+1

On 03/01/2018 11:59 AM, Roger Riggs wrote:

Hi Alan,

Thanks for the review. I added:

"There must be a colon and a SPACE after the name; the combined length will not 
exceed 72 characters."

Webrev updated in place:
   http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/

Thanks, Roger


On 2/26/2018 6:50 AM, Alan Bateman wrote:

On 23/02/2018 19:58, Roger Riggs wrote:

Please review this contribution from Philipp Kunz to handle manifest attribute 
names up to 70 bytes.

The change passes the available regression tests.
Manifest handling is somewhat sensitive so an additional review is appreciated.

Webrev: (rebased from the original patch of 2/22/18)
http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/

Issue:
https://bugs.openjdk.java.net/browse/JDK-6372077

I looked at the changes to the manifest code and I think they are okay. I don't 
have time to study the tests in detail.

I think it would be useful to copy the clarification note "(there must be a colon 
and a SPACE after the name)" from the JAR file spec to the equivalent sentence in 
the Attributes class description. That will help anyone wondering why the limit is 70 
rather than 72. This change shouldn't need a CSR as it's not adding any new assertions.

-Alan

Re: RFR JDK-8187653: Lock in CoderResult.Cache becomes performance bottleneck

[Alan Bateman]

On 01/03/2018 20:10, Xueming Shen wrote:


Right, they all should be final.
webrev has been updated accordingly.

Thanks, it looks okay now.

Re: JDK-6372077: JarFile.getManifest() should handle manifest attribute names up to 70 bytes

[Alan Bateman]

On 01/03/2018 19:59, Roger Riggs wrote:

Hi Alan,

Thanks for the review. I added:

"There must be a colon and a SPACE after the name; the combined length 
will not exceed 72 characters."


Webrev updated in place:
http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/

Thanks, looks okay to me now.

-Alan

Re: RFR: JDK-8197594: String#repeat

[Ivan Gerasimov]

Hi Jim!

I think the copying-loop can be slightly simplified, like this:

for (; copied < limit - copied; copied <<= 1) {
System.arraycopy(multiple, 0, multiple, copied, copied);
}

(didn't actually run it to check for correctness.)

With kind regards,
Ivan


On 2/28/18 8:31 AM, Jim Laskey wrote:

Introduction of a new instance method String::repeat to allow an efficient and 
concise approach for generating repeated character sequences as strings.

Performance information in JBS.

Thank you.

Cheers,

— Jim


JBS: https://bugs.openjdk.java.net/browse/JDK-8197594 

CSR: https://bugs.openjdk.java.net/browse/JDK-8198296
  
Webrev: http://cr.openjdk.java.net/~jlaskey/8197594/webrev-02/index.html 

JavaDoc: http://cr.openjdk.java.net/~jlaskey/8197594/String.html 







--
With kind regards,
Ivan Gerasimov

Re: RFR JDK-8187653: Lock in CoderResult.Cache becomes performance bottleneck

[Xueming Shen]

On 03/01/2018 12:00 PM, Alan Bateman wrote:



On 01/03/2018 19:42, Roger Riggs wrote:

Hi Sherman,

Looks to be a good fix with predictable behavior and performance.

+1, Roger

I assume malformed4 and unmappable4 should be final, the XXXCache fields too.

-Alan


Right, they all should be final.
webrev has been updated accordingly.

-sherman

Re: RFR JDK-8187653: Lock in CoderResult.Cache becomes performance bottleneck

[Alan Bateman]

On 01/03/2018 19:42, Roger Riggs wrote:

Hi Sherman,

Looks to be a good fix with predictable behavior and performance.

+1, Roger
I assume malformed4 and unmappable4 should be final, the XXXCache fields 
too.


-Alan

Re: JDK-6372077: JarFile.getManifest() should handle manifest attribute names up to 70 bytes

[Roger Riggs]

Hi Alan,

Thanks for the review. I added:

"There must be a colon and a SPACE after the name; the combined length 
will not exceed 72 characters."


Webrev updated in place:
   http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/

Thanks, Roger


On 2/26/2018 6:50 AM, Alan Bateman wrote:

On 23/02/2018 19:58, Roger Riggs wrote:
Please review this contribution from Philipp Kunz to handle manifest 
attribute names up to 70 bytes.


The change passes the available regression tests.
Manifest handling is somewhat sensitive so an additional review is 
appreciated.


Webrev: (rebased from the original patch of 2/22/18)
http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/

Issue:
https://bugs.openjdk.java.net/browse/JDK-6372077
I looked at the changes to the manifest code and I think they are 
okay. I don't have time to study the tests in detail.


I think it would be useful to copy the clarification note "(there must 
be a colon and a SPACE after the name)" from the JAR file spec to the 
equivalent sentence in the Attributes class description. That will 
help anyone wondering why the limit is 70 rather than 72. This change 
shouldn't need a CSR as it's not adding any new assertions.


-Alan

Re: 8198888: Reduce string allocation churninInvokerBytecodeGenerator

[Bernd Eckenfels]

Cool, the lastClass varient then is pretty tempting (and I wont fix the 
multi-slot Version below, it misses a „lastUsedSlot“ field)

Unrelated to this patch I feel that unifying internal names and external names 
will be a hard task, so Class#getInternalName() looks temting for a lot of 
similiar usecases mid term.

Gruss
Bernd
-- 
http://bernd.eckenfels.net

Von: Claes Redestad
Gesendet: Donnerstag, 1. März 2018 20:05
An: core-libs-dev@openjdk.java.net; e...@zusammenkunft.net
Betreff: Re: 819: Reduce string allocation churninInvokerBytecodeGenerator

Hi Bernd,

to dispel your concerns about keeping references to classes then 
InvokerBytecodeGenerators are shortlived and go out of scope after 
they've been used to generate an invoker (or LF) class, so the impl. 
here should be safe from a class leak perspective. A static cache is 
tempting, but would then of course require the use of weak (or soft) 
references and a concurrent collection, which might make it cost more 
than it'd save us.

It does seem the typical answer for the number of entries that get put 
into each generator's cache is exactly one, so a single entry cache 
isn't an unreasonable alternative here.

/Claes

On 2018-03-01 19:39, Bernd Eckenfels wrote:
> +if (nameMap == null) {
> +nameMap = new HashMap<>(2);
>
> What is the typical size the map ends up with? If it stays this small 
> linearly searching an Array (or „last value“) may be faster.
>
> If the map typically grows more than a few entries having some more initial 
> buckets will decrease collissions and avoid resizes. Especially considering 
> the fact that HashMap is a rather fat object, it does not hurt to start with 
> a bigger initial table. Does the lazy initialisation actually help anything?
>
> Single entry alternative
> 
> Object lastClass; String lastName;
>
> If (lastClass == c)
>return lastName;
> lastClass = c;
> lastName = c.getName().replace(‘.‘, ‘/‘);
> return lastName;
>
> (or make it a x Slot Version…):
> ---
> Object tab_obj = new tab_obj[8];
> String tab_val = new String[8];
>
> for(int i=0; i < tab_obj.length; i++)
> {
>If (tab_obj[i] == c);
>  return tab_val[i];
> }
> i = (i+1 % tab_obj.length);
> tab_obj[i] = c;
> return (tab_val[i] = c.getName().replace());
>
> (not sure about the concurrent semantics and having this static may actually 
> help)
>
> Gruss
> Bernd

Re: RFR JDK-8187653: Lock in CoderResult.Cache becomes performance bottleneck

[Roger Riggs]

Hi Sherman,

Looks to be a good fix with predictable behavior and performance.

+1, Roger

On 2/23/2018 8:26 PM, Xueming Shen wrote:

Hi,

Please help review the proposed change to remove the potential 
performance

bottleneck in CoderResult caused by the "over" synchronization the cache
mechanism.

issue: https://bugs.openjdk.java.net/browse/JDK-8187653
webrev: http://cr.openjdk.java.net/~sherman/8187653/webrev

Notes:
While the original test case (new String/String.getBytes() with UTF8 
as showed in the
stacktrace) described in the bug report might no longer be 
reproducible in jdk9, as
we have been optimizing lots of String related  char[]/byte[] coding 
path away from
the paths that use CoderResult. But it is still a concern/issue for 
the "general"
CharsetDe/Encoder.de/encode() operation, in which the malformed or 
unmappable

CoderResult object is returned.

As showed in the "[synchronized]" section in bm.scores [1], which is 
from the simple
jmh benchmark test CoderResultBM.java [2], the sores are getting 
significant worse

when the number of concurrent de/encoding threads gets bigger.

It appears the easy fix is to replace the sync mechanism from "method 
synchronized
+ HashMap" to  "ConcurrentHashMap" solves the problem, as showed in 
the same bm
result [1] in [ConcurrentHashMap] section, because most of the 
accesses to the caching
HashMap is read operation. The ConcurrentHahsMap's "almost non-block 
for retrieval

operation" really helps here.

There is another fact that might help us optimize further here. For 
most of our charsets
in JDK repository (with couple exceptions), the length of malformed 
and unmappable
CoderResult that these charsets might trigger actually is never longer 
than 4. So we might
not need to access the ConcurrentHashMap cache at all in normal use 
scenario. I'm putting
a CoderResult[4] on top the hashmap cache in proposed webrev. It does 
not improve the
performance significantly, but when the thread number gets bigger, a 
10%+ improvement
is observed. So I would assume it might be something worth doing, 
given its cost is really

low.

Thanks,
Sherman


[1] http://cr.openjdk.java.net/~sherman/8187653/bm.scores
[2] http://cr.openjdk.java.net/~sherman/8187653/CoderResultBM.java
[3] http://cr.openjdk.java.net/~sherman/8187653/bm.scores.ms932

Re: Oracle Java 8u161 regression in XML Schema Factory

[Joe Wang]

Hi Christoph and all,

Just wanted to let you know that we're in progress to update the release 
notes for 6u181/7u171/8u161 with a doc for this change.


Thanks,
Joe

On 2/22/2018 12:47 AM, Langer, Christoph wrote:

Hi Joe,

thanks for the clarification. It would be good to have a place of documentation 
where one could refer customers to.

Thanks
Christoph


-Original Message-
From: Joe Wang [mailto:huizhe.w...@oracle.com]
Sent: Mittwoch, 21. Februar 2018 21:50
To: Langer, Christoph 
Cc: Bernd ; OpenJDK Dev list 
Subject: Re: Oracle Java 8u161 regression in XML Schema Factory



@Joe: Is there some documentation for this change in default behavior

that came with JDK8u161? I assume it is for higher security and cannot be
reverted (e.g. by setting the feature default for
Djdk.xml.overrideDefaultParser to true).

It is indeed. It was a customer's request. Customers' complaints were
that a factory created through the official API could in many cases,
unknowingly to the customers, load 3rd party parsers that didn't
necessarily implement the security features added since JDK7u40 and 8.
For customers, this behavior was a security concern. It was particularly
inconvenient to users who might have some 3rd party libraries that just
happen to be in their environment.

This change was not mentioned in the release notes. I'll check whether
we could find a right place for a note of this change. The 8u161 release
notes would have been a good place to do so.

Best,
Joe


Best regards
Christoph


-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On
Behalf Of Bernd
Sent: Dienstag, 13. Februar 2018 22:55
To: OpenJDK Dev list
Subject: Re: Oracle Java 8u161 regression in XML Schema Factory

Hello,


2018-01-25 17:41 GMT+01:00 Seán Coffey:


Classes nearer to those below were touched via JDK-8186080: Transform

XML

interfaces
http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/cb84156d54b2
http://hg.openjdk.java.net/jdk8u/jdk8u/jaxp/rev/08a44c164993

This may be connected with some tools trying to redefine the classes
perhaps. Needs more investigating. Perhaps the XMLSchemaLoader

changes are

a factor ?


I have ben able to extract a minimal reproducer. It is not related to
XMLUnit, only to powermock. If it instruments com.sun but not javax.xml
(and other combinations) then it fails.

For details see the readme in this maven project:

https://github.com/ecki/reproduce-schemaex

I also found a way to make it work with both versions, so its no longer an
issue for me, but there is definitely some changes (which might also be
triggered in AppServers or OSGi containers with partially reconfigured
implementations. Not sure if you want to investigate deeper).

Gruss
Bernd


Here is the stacktrace anyway:

com.sun.org.apache.xerces.internal.impl.dv.DVFactoryException:

Schema

factory class
com.sun.org.apache.xerces.internal.impl.dv.xs.SchemaDVFactoryImpl

does

not
extend from SchemaDVFactory.
   at
com.sun.org.apache.xerces.internal.impl.dv.SchemaDVFactory.
getInstance(SchemaDVFactory.java:75)
   at
com.sun.org.apache.xerces.internal.impl.dv.SchemaDVFactory.
getInstance(SchemaDVFactory.java:57)
   at
com.sun.org.apache.xerces.internal.impl.xs.XMLSchemaLoader.
reset(XMLSchemaLoader.java:1024)
   at
com.sun.org.apache.xerces.internal.impl.xs.XMLSchemaLoader.
loadGrammar(XMLSchemaLoader.java:556)
   at
com.sun.org.apache.xerces.internal.impl.xs.XMLSchemaLoader.
loadGrammar(XMLSchemaLoader.java:535)
   at
com.sun.org.apache.xerces.internal.jaxp.validation.XMLSchema
Factory.newSchema(XMLSchemaFactory.java:254)
   at javax.xml.validation.SchemaFactory.newSchema(SchemaFactory.
java:638)
   at javax.xml.validation.SchemaFactory.newSchema(SchemaFactory.
java:654)
   at
com.seeburger.api.test.helpers.BuilderTestHelper.getCRSchema
Validator(BuilderTestHelper.java:57)
   at
com.seeburger.api.test.helpers.BuilderTestHelper.validateAnd
Compare(BuilderTestHelper.java:73)
   at
com.seeburger.api.test.KSMBuilderTest.testDeletePGP(KSMBuild
erTest.java:196)
   at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAcce
ssorImpl.java:62)
   at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMe
thodAccessorImpl.java:43)
   at java.lang.reflect.Method.invoke(Method.java:498)
   at

org.junit.internal.runners.TestMethod.invoke(TestMethod.java:68)

   at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44R


unnerDelegateImpl$PowerMockJUnit44MethodRunner.runTestMethod

(PowerMockJUnit44RunnerDelegateImpl.java:310)
   at org.junit.internal.runners.MethodRoadie$2.run(MethodRoadie.
java:89)
   at
org.junit.internal.runners.MethodRoadie.runBeforesThenTestTh
enAfters(MethodRoadie.java:97)
   at

Re: 8198888: Reduce string allocation churn inInvokerBytecodeGenerator

[Claes Redestad]

Hi Bernd,

to dispel your concerns about keeping references to classes then 
InvokerBytecodeGenerators are shortlived and go out of scope after 
they've been used to generate an invoker (or LF) class, so the impl. 
here should be safe from a class leak perspective. A static cache is 
tempting, but would then of course require the use of weak (or soft) 
references and a concurrent collection, which might make it cost more 
than it'd save us.


It does seem the typical answer for the number of entries that get put 
into each generator's cache is exactly one, so a single entry cache 
isn't an unreasonable alternative here.


/Claes

On 2018-03-01 19:39, Bernd Eckenfels wrote:

+if (nameMap == null) {
+nameMap = new HashMap<>(2);

What is the typical size the map ends up with? If it stays this small linearly 
searching an Array (or „last value“) may be faster.

If the map typically grows more than a few entries having some more initial 
buckets will decrease collissions and avoid resizes. Especially considering the 
fact that HashMap is a rather fat object, it does not hurt to start with a 
bigger initial table. Does the lazy initialisation actually help anything?

Single entry alternative

Object lastClass; String lastName;

If (lastClass == c)
   return lastName;
lastClass = c;
lastName = c.getName().replace(‘.‘, ‘/‘);
return lastName;

(or make it a x Slot Version…):
---
Object tab_obj = new tab_obj[8];
String tab_val = new String[8];

for(int i=0; i < tab_obj.length; i++)
{
   If (tab_obj[i] == c);
 return tab_val[i];
}
i = (i+1 % tab_obj.length);
tab_obj[i] = c;
return (tab_val[i] = c.getName().replace());

(not sure about the concurrent semantics and having this static may actually 
help)

Gruss
Bernd

Re: RFR: 8198888: Reduce string allocation churn inInvokerBytecodeGenerator

[Bernd Eckenfels]

Another question is it safe to keep an unbounded strong reference to classes 
here? Maybe better Cache getName() results?

-- 
http://bernd.eckenfels.net

Von: Paul Sandoz
Gesendet: Donnerstag, 1. März 2018 19:05
An: Claes Redestad
Cc: core-libs-dev
Betreff: Re: RFR: 819: Reduce string allocation churn 
inInvokerBytecodeGenerator



> On Mar 1, 2018, at 4:14 AM, Claes Redestad  wrote:
> 
> Hi,
> 
> two trivial optimizations that get rid of a few percent on ISC startup tests.
> 
> Webrev: http://cr.openjdk.java.net/~redestad/819/jdk.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-819
> 

Looks good. 

Unsurprising that int is so common given it’s the basic carrier for other 
primitive types.

Paul.

Re: [JDK 11] RFR 8198821: fix test methods access for test java/text/Normalizer/NormalizerAPITest.java

[Naoto Sato]

+1

Naoto

On 2/28/18 6:03 PM, Chris Yin wrote:

Please review the minor change for test 
java/text/Normalizer/NormalizerAPITest.java, thanks

Added public access modifier to all “Test_" methods so they can be recognized 
as test method correctly by util class

bug: https://bugs.openjdk.java.net/browse/JDK-8198821 

webrev: http://cr.openjdk.java.net/~xiaofeya/8198821/webrev.00/ 


Regards,
Chris

Re: RFR: 8198888: Reduce string allocation churn in InvokerBytecodeGenerator

[Paul Sandoz]

It’s not just the internal VM name, it's also a descriptor in byte code.

I hope the symbolic ref API currently being designed in the amber repo will 
help here. Until that settles i would be reluctant to consider adding public 
“toDescriptor” methods, or even do something internally right now if we can 
sweep it all up later on.

(Further, the symbolic ref API may also be an opportunity to clean up the 
parsing side of things too.)
 
Pau.

> On Mar 1, 2018, at 5:31 AM, Claes Redestad  wrote:
> 
> That few, huh? Well, if you ask me the VM internal name and the java class 
> names should have been synced up long ago. :-)
> 
> I'd not be averse to some real API point in a future RFE. I think this has 
> been dodged in the past to avoid leaking the VM internal representation 
> everywhere.
> 
> /Claes
> 
> On 2018-03-01 14:17, Peter Levart wrote:
>> Hi Claes,
>> 
>>  623 name = c.getName().replace('.', '/');
>> 
>> Did you know that there are 319 occurrences of .replace('.', '/') in JDK 
>> sources and tests (45 occurrences in java.base without tests)?
>> 
>> Isn't that enough to create a special API on the java.lang.Class that would 
>> cache this or at least something in an internal java.base package?
>> 
>> Regards, Peter
>> 
>> On 03/01/2018 01:14 PM, Claes Redestad wrote:
>>> Hi,
>>> 
>>> two trivial optimizations that get rid of a few percent on ISC startup 
>>> tests.
>>> 
>>> Webrev: http://cr.openjdk.java.net/~redestad/819/jdk.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-819
>>> 
>>> Thanks!
>>> 
>>> /Claes
>> 
> 

Re: RFR: 8198888: Reduce string allocation churn in InvokerBytecodeGenerator

[Paul Sandoz]

> On Mar 1, 2018, at 4:14 AM, Claes Redestad  wrote:
> 
> Hi,
> 
> two trivial optimizations that get rid of a few percent on ISC startup tests.
> 
> Webrev: http://cr.openjdk.java.net/~redestad/819/jdk.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-819
> 

Looks good. 

Unsurprising that int is so common given it’s the basic carrier for other 
primitive types.

Paul.

Re: RFR JDK-8197533 move javax.transaction.xa into its own module

[Paul Sandoz]

+1

> On Mar 1, 2018, at 8:59 AM, Lance Andersen  wrote:
>> 
>> +1, i second Joe’s request to update package-info.java while we are 
>> opportunistically cleaning this area up.
> 
> Per your and Joe’s request, please see 
> cr.openjdk.java.net/~lancea/8197533/webrev.02 
>  which has your 
> suggested update to use package-info.java
> 
> Thank you both
> 
> Best
> Lance

Re: RFR: JDK-8197594: String#repeat

[Paul Sandoz]

> On Mar 1, 2018, at 6:12 AM, Jim Laskey  wrote:
> 
> Peter,
> 
> Very reasonable and worth considering. The main reason the power of 2 copy 
> works well is that the source is (almost) always cached.
> 

That was my thinking too, the current approach is fine for anticipated string 
sizes and repeats. 

We could consider more optimal and general cache friendly approaches but the 
API cost is likely higher than the benefit e.g. if we add a method to Arrays 
then 8 + 9 others (all primitives + ref and offset accepting methods) tend to 
come along for the ride.

Paul.

> I thought about this a bit at the beginning and wondered about introducing 
> Arrays.fill(T[] dst, T[] src) where dst is filled repeatedly from src. This 
> can then become an intrinsic and “do the right thing” based on hardware. 
> String::repeat can be adapted later. I didn’t pursue because I lacked other 
> use cases.
> 
> File a performance against the implementation (once checked in) and we can 
> see what the performance team thinks.
> 

> Cheers,
> 
> — Jim

Re: RFR JDK-8197533 move javax.transaction.xa into its own module

[Lance Andersen]

> On Feb 28, 2018, at 8:29 PM, Paul Sandoz  wrote:
> 
> 
> 
>> On Feb 28, 2018, at 1:22 PM, Lance Andersen > > wrote:
>> 
>>> 
>>> On Feb 28, 2018, at 2:20 PM, Lance Andersen >> > wrote:
>>> 
>>> Hi Paul,
>>> 
>>> Thank you for the review.
 On Feb 28, 2018, at 1:40 PM, Paul Sandoz > wrote:
 
 Compatible module refactoring in action!
 
 Looks good, one comment:
 
 test/jdk/javax/transaction/xa/testng/JavaSqlModuleDriver.java
 
 This is not a valid Java source file can you merge the jtreg meta data 
 into XAExceptionTests instead?
>>> 
>>> As we discussed offline, I will change the above file and Driver.java.  I 
>>> naively assumed this was OK as the change  to add Driver.java was made 
>>> after I had originally added these tests in 2015.
>> 
>> http://cr.openjdk.java.net/~lancea/8197533/webrev.01/ 
>>  has the updated tests
> 
> +1, i second Joe’s request to update package-info.java while we are 
> opportunistically cleaning this area up.

Per your and Joe’s request, please see 
cr.openjdk.java.net/~lancea/8197533/webrev.02 which has your suggested update 
to use package-info.java

Thank you both

Best
Lance
> 
> Paul.

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: JDK-8197594: String#repeat

[Jim Laskey]

Thank you Bernd.

> On Mar 1, 2018, at 11:04 AM, Bernd Eckenfels  wrote:
> 
> Hello,
>  
> I would not encourage makeshift number formatting by mentioning left padding 
> as a major usecase:
>  
> Just remove that part:
>  
> + * 
> + * This method may be used to create space padding for
> + * formatting text or zero padding for formatting numbers.
>  
> I think this error without an Explanation can be hard to debug. It should be 
> clear that Memory allocation was not attempted and what the actual Parameters 
> where:
>  
> +if (Integer.MAX_VALUE / count < len) {
> +throw new OutOfMemoryError();
> +}
>  
> „Repeating „ + len + „ bytes „ + count + „ times would produce a String 
> exceeding maximum size.“
>  
> Gruss
> Bernd
> -- 
> http://bernd.eckenfels.net 
>  
> Von: Jim Laskey 
> Gesendet: Mittwoch, 28. Februar 2018 21:01
> An: Core-Libs-Dev 
> Betreff: RFR: JDK-8197594: String#repeat
>  
> Introduction of a new instance method String::repeat to allow an efficient 
> and concise approach for generating repeated character sequences as strings.
>  
> Performance information in JBS.
>  
> Thank you.
>  
> Cheers,
>  
> — Jim
>  
>  
> JBS: https://bugs.openjdk.java.net/browse/JDK-8197594 
> 
> CSR: https://bugs.openjdk.java.net/browse/JDK-8198296
> 
> Webrev: http://cr.openjdk.java.net/~jlaskey/8197594/webrev-02/index.html 
> 
> JavaDoc: http://cr.openjdk.java.net/~jlaskey/8197594/String.html 
> 

Re: JDK-8197594: String#repeat

[Bernd Eckenfels]

Hello,

I would not encourage makeshift number formatting by mentioning left padding as 
a major usecase:

Just remove that part:

+ * 
+ * This method may be used to create space padding for
+ * formatting text or zero padding for formatting numbers.

I think this error without an Explanation can be hard to debug. It should be 
clear that Memory allocation was not attempted and what the actual Parameters 
where:

+if (Integer.MAX_VALUE / count < len) {
+throw new OutOfMemoryError();
+}

„Repeating „ + len + „ bytes „ + count + „ times would produce a String 
exceeding maximum size.“

Gruss
Bernd
-- 
http://bernd.eckenfels.net

Von: Jim Laskey
Gesendet: Mittwoch, 28. Februar 2018 21:01
An: Core-Libs-Dev
Betreff: RFR: JDK-8197594: String#repeat

Introduction of a new instance method String::repeat to allow an efficient and 
concise approach for generating repeated character sequences as strings.

Performance information in JBS.

Thank you.

Cheers,

— Jim


JBS: https://bugs.openjdk.java.net/browse/JDK-8197594 

CSR: https://bugs.openjdk.java.net/browse/JDK-8198296
 
Webrev: http://cr.openjdk.java.net/~jlaskey/8197594/webrev-02/index.html 

JavaDoc: http://cr.openjdk.java.net/~jlaskey/8197594/String.html 

RFR: JDK-8144300 : Java does not honor http.nonProxyHosts value having wildcard * both at end and start

[Pallavi Sonal]

Hi,

Please review the following change to fix JDK-8144300. 

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8144300 

Webrev : http://cr.openjdk.java.net/~rpatil/8144300/webrev.00/ 

 

Whenever there is a wildcard(*) at either the beginning or the end of a 
hostname specified in the list of http.nonProxyHosts , the code works as 
expected and DIRECT connection is used for them. But in scenarios, where there 
is a wildcard both at the beginning and end of the hostname, proxy is used 
instead of DIRECT connection.

 

All the tier1 and tier2 Mach 5 tests have passed.

 

Thanks,

Pallavi Sonal

Re: RFR: JDK-8197594: String#repeat

[Claes Redestad]

Yes,

I think you can squeeze out a 10% improvement or so on some combinations of
shortness of String and lowness of repeat count, but at the cost of adding
another branch, risking costs due to profile pollution, decreasing 
chance the
method gets inlined etc.. it didn't seem worth the trouble for something 
that

already is a much larger performance improvement than so over existing
alternatives.

Thanks!

/Claes

On 2018-03-01 15:15, Jim Laskey wrote:

Roger,

Claes did some performance tests against an earlier implementation where I was 
doing just that. We determined that the performance difference was lost in the 
noise.

Cheers,

— Jim





On Mar 1, 2018, at 10:11 AM, Roger Riggs  wrote:

Hi Jim,

Perhaps premature optimization...

Have you done any JMH tests on the expected string content and sizes.  The 
optimization for single 8-bit characters
is good but for short strings like inserting spaces for tab stops, "
".repeat(5), will suffer the overhead of Arraycopy
for very small/short or non-8-bit strings.   Perhaps use a regular loop below some 
threshold ( limit < 32).
Maybe the intrinsic already handles this efficiently or it is not important 
enough to optimize.

I would not expect to see many very long repeats or long strings being repeated.
And I would leave cache-effect optimizations to the implementation of Arraycopy.

Thanks, Roger

On 2/28/2018 9:13 PM, Paul Sandoz wrote:

Hi Jim,

Looks good. I like the power of 2 copying.


2978  * @throws  IllegalArgumentException if the {@code count} is
2979  *  negative.
2980  */
2981 public String repeat(int count) {

Missing @since11 on the method.


Like Stuart suggests, turn the explanatory text into an api note, perhaps with 
a small code sample.

Thanks,
Paul.
  

On Feb 28, 2018, at 8:31 AM, Jim Laskey  wrote:

Introduction of a new instance method String::repeat to allow an efficient and 
concise approach for generating repeated character sequences as strings.

Performance information in JBS.

Thank you.

Cheers,

— Jim


JBS: https://bugs.openjdk.java.net/browse/JDK-8197594 

CSR: https://bugs.openjdk.java.net/browse/JDK-8198296

Webrev: http://cr.openjdk.java.net/~jlaskey/8197594/webrev-02/index.html 

JavaDoc: http://cr.openjdk.java.net/~jlaskey/8197594/String.html 

Re: RFR: JDK-8197594: String#repeat

[Jim Laskey]

Roger,

Claes did some performance tests against an earlier implementation where I was 
doing just that. We determined that the performance difference was lost in the 
noise.

Cheers,

— Jim




> On Mar 1, 2018, at 10:11 AM, Roger Riggs  wrote:
> 
> Hi Jim,
> 
> Perhaps premature optimization...
> 
> Have you done any JMH tests on the expected string content and sizes.  The 
> optimization for single 8-bit characters
> is good but for short strings like inserting spaces for tab stops, "
> ".repeat(5), will suffer the overhead of Arraycopy
> for very small/short or non-8-bit strings.   Perhaps use a regular loop below 
> some threshold ( limit < 32).
> Maybe the intrinsic already handles this efficiently or it is not important 
> enough to optimize.
> 
> I would not expect to see many very long repeats or long strings being 
> repeated.
> And I would leave cache-effect optimizations to the implementation of 
> Arraycopy.
> 
> Thanks, Roger
> 
> On 2/28/2018 9:13 PM, Paul Sandoz wrote:
>> Hi Jim,
>> 
>> Looks good. I like the power of 2 copying.
>> 
>> 
>> 2978  * @throws  IllegalArgumentException if the {@code count} is
>> 2979  *  negative.
>> 2980  */
>> 2981 public String repeat(int count) {
>> 
>> Missing @since11 on the method.
>> 
>> 
>> Like Stuart suggests, turn the explanatory text into an api note, perhaps 
>> with a small code sample.
>> 
>> Thanks,
>> Paul.
>>  
>>> On Feb 28, 2018, at 8:31 AM, Jim Laskey  wrote:
>>> 
>>> Introduction of a new instance method String::repeat to allow an efficient 
>>> and concise approach for generating repeated character sequences as strings.
>>> 
>>> Performance information in JBS.
>>> 
>>> Thank you.
>>> 
>>> Cheers,
>>> 
>>> — Jim
>>> 
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8197594 
>>> 
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8198296
>>> 
>>> Webrev: http://cr.openjdk.java.net/~jlaskey/8197594/webrev-02/index.html 
>>> 
>>> JavaDoc: http://cr.openjdk.java.net/~jlaskey/8197594/String.html 
>>> 
>>> 
>>> 
>>> 
> 

Re: RFR: JDK-8197594: String#repeat

[Jim Laskey]

Peter,

Very reasonable and worth considering. The main reason the power of 2 copy 
works well is that the source is (almost) always cached.

I thought about this a bit at the beginning and wondered about introducing 
Arrays.fill(T[] dst, T[] src) where dst is filled repeatedly from src. This can 
then become an intrinsic and “do the right thing” based on hardware. 
String::repeat can be adapted later. I didn’t pursue because I lacked other use 
cases.

File a performance against the implementation (once checked in) and we can see 
what the performance team thinks.

Cheers,

— Jim


> On Mar 1, 2018, at 7:50 AM, Peter Levart  wrote:
> 
> Hi,
> 
> On 03/01/2018 03:13 AM, Paul Sandoz wrote:
>> Hi Jim,
>> 
>> Looks good. I like the power of 2 copying.
> 
> Is this really the fastest way? Say you are doing this:
> 
> String s = ... 64 bytes ...
> 
> s.repeat(16384);
> 
> ...the last arraycopy will be copying 512 KiB from one part of memory to the 
> other part. It means that the source 512 KiB range will not fit into L1 
> cache. Neither fully into L2 cache.
> 
> The fastest way might be to employ power-of-two copying until the range 
> reaches L1 caches size / 2 for example (16 K ?) and then use the same source 
> range repeatedly as a "stamp" until the rounded end.
> 
> What do you think?
> 
> Regards, Peter
> 

Re: RFR: JDK-8197594: String#repeat

[Roger Riggs]

Hi Jim,

Perhaps premature optimization...

Have you done any JMH tests on the expected string content and sizes.  
The optimization for single 8-bit characters
is good but for short strings like inserting spaces for tab stops, "    
".repeat(5), will suffer the overhead of Arraycopy
for very small/short or non-8-bit strings.   Perhaps use a regular loop 
below some threshold ( limit < 32).
Maybe the intrinsic already handles this efficiently or it is not 
important enough to optimize.


I would not expect to see many very long repeats or long strings being 
repeated.
And I would leave cache-effect optimizations to the implementation of 
Arraycopy.


Thanks, Roger

On 2/28/2018 9:13 PM, Paul Sandoz wrote:

Hi Jim,

Looks good. I like the power of 2 copying.


2978  * @throws  IllegalArgumentException if the {@code count} is
2979  *  negative.
2980  */
2981 public String repeat(int count) {

Missing @since11 on the method.


Like Stuart suggests, turn the explanatory text into an api note, perhaps with 
a small code sample.

Thanks,
Paul.
  


On Feb 28, 2018, at 8:31 AM, Jim Laskey  wrote:

Introduction of a new instance method String::repeat to allow an efficient and 
concise approach for generating repeated character sequences as strings.

Performance information in JBS.

Thank you.

Cheers,

— Jim


JBS: https://bugs.openjdk.java.net/browse/JDK-8197594 

CSR: https://bugs.openjdk.java.net/browse/JDK-8198296

Webrev: http://cr.openjdk.java.net/~jlaskey/8197594/webrev-02/index.html 

JavaDoc: http://cr.openjdk.java.net/~jlaskey/8197594/String.html 

Re: RFR: 8198888: Reduce string allocation churn in InvokerBytecodeGenerator

[Claes Redestad]

That few, huh? Well, if you ask me the VM internal name and the java 
class names should have been synced up long ago. :-)


I'd not be averse to some real API point in a future RFE. I think this 
has been dodged in the past to avoid leaking the VM internal 
representation everywhere.


/Claes

On 2018-03-01 14:17, Peter Levart wrote:

Hi Claes,

 623 name = c.getName().replace('.', '/');

Did you know that there are 319 occurrences of .replace('.', '/') in 
JDK sources and tests (45 occurrences in java.base without tests)?


Isn't that enough to create a special API on the java.lang.Class that 
would cache this or at least something in an internal java.base package?


Regards, Peter

On 03/01/2018 01:14 PM, Claes Redestad wrote:

Hi,

two trivial optimizations that get rid of a few percent on ISC 
startup tests.


Webrev: http://cr.openjdk.java.net/~redestad/819/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-819

Thanks!

/Claes

Re: RFR: 8198888: Reduce string allocation churn in InvokerBytecodeGenerator

[Peter Levart]

Hi Claes,

 623 name = c.getName().replace('.', '/');

Did you know that there are 319 occurrences of .replace('.', '/') in JDK 
sources and tests (45 occurrences in java.base without tests)?


Isn't that enough to create a special API on the java.lang.Class that 
would cache this or at least something in an internal java.base package?


Regards, Peter

On 03/01/2018 01:14 PM, Claes Redestad wrote:

Hi,

two trivial optimizations that get rid of a few percent on ISC startup 
tests.


Webrev: http://cr.openjdk.java.net/~redestad/819/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-819

Thanks!

/Claes

Re: RFR: 8198888: Reduce string allocation churn in InvokerBytecodeGenerator

[Claes Redestad]

Hi,

as equals/hashCode on Class mirrors are already using identity 
semantics, I don't
think we actually gain much from using an IdentityHashMap, and I don't 
want to add
new dependencies to this java.lang.invoke (we have some ambition to 
reduce the
number of java.util classes used from java.lang.invoke so as to make it 
less

troublesome to use lambdas et al from java.util).

Thanks!

/Claes

On 2018-03-01 13:31, Andrej Golovnin wrote:

Hi Claes,

src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java

   96 /** Internal name lookup cache. */
   97 private HashMap nameMap;

617 nameMap = new HashMap<>(2);

Have you tested it with IdentityHashMap? Maybe it would be even better
as Class does not have own implementations of #hasCode() and
#equals() methods.

Best regards,
Andrej Golovnin

On Thu, Mar 1, 2018 at 1:14 PM, Claes Redestad
 wrote:

Hi,

two trivial optimizations that get rid of a few percent on ISC startup
tests.

Webrev: http://cr.openjdk.java.net/~redestad/819/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-819

Thanks!

/Claes

Re: RFR: 8198888: Reduce string allocation churn in InvokerBytecodeGenerator

[Andrej Golovnin]

Hi Claes,

src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java

  96 /** Internal name lookup cache. */
  97 private HashMap nameMap;

617 nameMap = new HashMap<>(2);

Have you tested it with IdentityHashMap? Maybe it would be even better
as Class does not have own implementations of #hasCode() and
#equals() methods.

Best regards,
Andrej Golovnin

On Thu, Mar 1, 2018 at 1:14 PM, Claes Redestad
 wrote:
> Hi,
>
> two trivial optimizations that get rid of a few percent on ISC startup
> tests.
>
> Webrev: http://cr.openjdk.java.net/~redestad/819/jdk.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-819
>
> Thanks!
>
> /Claes

RFR: 8198888: Reduce string allocation churn in InvokerBytecodeGenerator

[Claes Redestad]

Hi,

two trivial optimizations that get rid of a few percent on ISC startup 
tests.


Webrev: http://cr.openjdk.java.net/~redestad/819/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-819

Thanks!

/Claes

Re: RFR: JDK-8197594: String#repeat

[Peter Levart]

Hi,

On 03/01/2018 03:13 AM, Paul Sandoz wrote:

Hi Jim,

Looks good. I like the power of 2 copying.


Is this really the fastest way? Say you are doing this:

String s = ... 64 bytes ...

s.repeat(16384);

...the last arraycopy will be copying 512 KiB from one part of memory to 
the other part. It means that the source 512 KiB range will not fit into 
L1 cache. Neither fully into L2 cache.


The fastest way might be to employ power-of-two copying until the range 
reaches L1 caches size / 2 for example (16 K ?) and then use the same 
source range repeatedly as a "stamp" until the rounded end.


What do you think?

Regards, Peter