Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

[David Holmes]

On 29/07/2016 5:55 PM, Leela Mohan wrote:

Hi David,

I understand, Klass types are no longer oops
 but JNIHandles::resolve_non_null() would expose naked oops. In other
words, KlassOops are no longer oops but java.lang.Class objects are.


Yes my mistake - focusing on the wrong aspect. Good catch.

Thanks,
David


Thanks,
Leela

On Thu, Jul 28, 2016 at 10:51 PM, David Holmes > wrote:

Hi Leela,

On 29/07/2016 12:59 PM, Leela Mohan wrote:

I think, change in the file unsafe.cpp is incorrect. (
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/ )

Below function is accessing naked oops when thread has
transitioned to
"native":

*+ static jobject get_class_loader(JNIEnv* env, jclass cls)
{**+   if
(java_lang_Class::is_primitive(JNIHandles::resolve_non_null(cls)))
{**+ return NULL;**+   }**+   Klass* k =
java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));**+   oop
loader = k->class_loader();**+   return JNIHandles::make_local(env,
loader);**+ }*


klass types are no longer oops in the Java heap, but metaspace
objects that reside in a per-classloader allocation region. They
never get compacted or relocated so raw pointers to them are safe.

Cheers,
David



- Leela

On Mon, Sep 8, 2014 at 7:05 PM, Coleen Phillimore <
coleen.phillim...@oracle.com
> wrote:


Thanks, Mandy!

Coleen


On 9/8/14, 6:59 PM, Mandy Chung wrote:

Thumbs up.

Mandy

On 9/5/2014 12:55 PM, Coleen Phillimore wrote:

Summary: Add classLoader to java/lang/Class instance
for fast access

This is a backport request for 8u40.   This change
has been in the jdk9
code for 3 months without any problems.

The JDK changes hg imported cleanly.  The Hotspot
change needed a hand
merge for create_mirror call in klass.cpp.

http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

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

Ran jdk_core jtreg tests in jdk with both
jdk/hotspot changes. Also ran
jck java_lang tests with only the hotspot change.
The hotspot change can
be tested separately from the jdk change (but not
the other way around).

Thanks,
Coleen

Re: Change of the toString implementation for annotations

[joe darcy]

It appears the plain overloads of Arrays.toString cannot be used to 
provide an annotation source compatible form since they use "[]" rather 
than "{}" to surround to contents. I'll see if I can handle that.


On 7/28/2016 8:33 AM, Rafael Winterhalter wrote:
Another remark about "printability". Array types are printed in their 
internal form, i.e. "java.lang.Void[].class" becomes 
"Ljava.lang.Void;.class" in the string form.


Ah yes, good catch -- some more checking is required there.



While I very much agree over the improvements of the toString 
implementation of the unresolved values, I still wonder if this 
special treatment is really necessary and justify the compatibility 
break. Are the curly braces meant to avoid confusion from parsing 
annotation values where the braces could also be part of a type 
literal? Any parser would need to process the annotation string 
sensitively anyways as string values could contain any value, if this 
is the case.


As a general comment, the exact toString output of an annotation is 
deliberately not specified; from java.lang.annotation.Annotation.toString():


 * Returns a string representation of this annotation.  The details
 * of the representation are implementation-dependent [...]

Therefore, the details of the toString output of annotations are not a 
formally exported interface of the platform. (For more details 
discussions of such matters see [1].)


Early in the life of the platform, there was a preference to exactly 
specify toString output and hash values, etc., but this policy was found 
to be overly constraining so we moved away from that.


HTH,

-Joe

[1] 
http://cr.openjdk.java.net/~darcy/OpenJdkDevGuide/OpenJdkDevelopersGuide.v0.777.html




Best regards, Rafael

2016-07-28 15:38 GMT+02:00 Rafael Winterhalter >:


Hi Joe,
thank you for your answer. Can I ask for the rationale of using {}
instead of [] only for classes? If the goal was to come closer to
the source code representation, would this not imply to use curly
braces for all array of an annotation?
Thank you for your time! Best regards, Rafael

2016-07-19 4:13 GMT+02:00 joe darcy >:

Hello Rafael,

On 7/18/2016 5:43 PM, Rafael Winterhalter wrote:

Hello,
I recognized a failing test on Java 9 caused by a changed
return value
returned by toString on an annotation with a class-property.

When calling toString on an annotation: @interface Foo {
Class value();
} instantiated as @Foo(Bar.class) with Java 8 it would be
printed as:

@Foo(class Bar)

while in Java 9 it is printed as:

@Foo(Bar.class)

Is this change intended? I do not see a big benefit of
this implementation
change and it could break code. In my case, the problem is
not that big, it
is an easy fix but still, I found it a bit surprising.


I pushed the change your test noticed; it was done as part of

JDK-5040830: (ann) please improve toString() for
annotations containing exception proxies
https://bugs.openjdk.java.net/browse/JDK-5040830

The basic rationale for the change is that "Foo.class" is the
syntax that appears when annotations are in source code so the
toString() form should generally reflect that.

Thanks for running your project against JDK 9 builds; HTH,

-Joe

Re: java.io.Writer uses CharSequence.toString()

[Brent Christian]

Hi,

This idea has been brought up before [1].

I concur with Pavel's assessment.  I would add that now that latin-1 
Strings are stored in a more compact form in JDK 9 ("Compact Strings" 
[2]), the performance profile of string data is further complicated.


Thanks,
-Brent

1. https://bugs.openjdk.java.net/browse/JDK-6206838
2. https://bugs.openjdk.java.net/browse/JDK-8054307
On 07/29/2016 10:21 AM, Pavel Rappo wrote:

Once again, while I agree in some places it could have been done a bit better
probably, I would say it's good to a have a look at benchmarks first.

If they show there's indeed a big difference between

char[] copy = new chars[charSequence.length()];
String s = charSequence.toString();
s.getChars(0, s.length, copy, 0);

and

char[] copy = new chars[charSequence.length()];
charSequence.getChars(0, charSequence.length(), copy, 0);

it could justify an increase in complexity of CharBuffer.append or introducing a
new default method (getChars/fillInto) into CharSequence. Possibly. Or maybe
not. Because there might be some nontrivial effects we are completely unaware 
of.

Btw, what do you mean by "extract char[]" from StringBuilder? Do you want
StringBuilder to give away a reference to its char[] outside? If not, than
what's the difference between "extract char[]" from StringBuilder and "use
String" in your algorithm?

The bottom line is whatever you suggest would likely need a good justification.
To me it's not immediately obvious that something like this

 public CharBuffer append(CharSequence csq) {
 if (csq == null) {
 put("null");
 } else if (csq instanceof StringBuilder) {
 char[] chars = new char[csq.length()];
 ((StringBuilder) csq).getChars(0, csq.length(), chars, 0);
 put(chars);
 } else if (csq instanceof StringBuffer) {
 char[] chars = new char[csq.length()];
 ((StringBuffer) csq).getChars(0, csq.length(), chars, 0);
 put(chars);
 } else if (csq instanceof CharBuffer) {
 CharBuffer buffer = (CharBuffer) csq;
 int p = buffer.position();
 put(buffer);
 buffer.position(p);
 } else {
 for (int i = 0; i < csq.length(); i++) {
 put(csq.charAt(i));
 }
 }
 return this;
 }

is better than this (what's there today)

 public CharBuffer append(CharSequence csq) {
 if (csq == null)
 return put("null");
 else
 return put(csq.toString());
 }


On 29 Jul 2016, at 15:12, e...@zusammenkunft.net wrote:

Hello,

Have to agree with Fabian handling CharSequences (and special case 
StringBuilder) is pretty weak, in CharBuffer.append(CharSequence) you see the 
same toString. I would expect it to do:
- Instamceof String -> use it
- Instance of StringBuilder -> extract char[] and iterate
- Instance of CharBuffer -> handle
- Otherwise: Loop over charAt

(the otherwise might be a tradeof between allocation and (not)inlined bounds 
checks)

Alternative would be a CharSequence.fillInto(char[])

BTW wouldn't it be create if char[] implements CharSequence?

Gruss
Bernd
--
http://bernd.eckenfels.net
 From Win 10 Mobile

Von: Fabian Lange

Re: Files.read/readAllBytes can loop once with zero size buffer

[Bernd Eckenfels]

Hello,

BTW: I think accessing named pipes/fifos would have the same "problem".
But I guess nobody would use readAllBytes() on them :)

Gruss
Bernd


Am Fri, 29 Jul 2016 21:46:03 +0100
schrieb Alan Bateman :

> On 29/07/2016 19:36, Martin Buchholz wrote:
> 
> > I think keeping the status quo is the right outcome.  The /proc
> > filesystem is intentionally providing incorrect information because
> > providing correct information is deemed to be too expensive (have
> > to compute contents of file for stat?).  Reading /proc is
> > sufficiently expensive that the extra realloc of an empty buffer is
> > noise.  Be thankful that reading files in /proc works at all!
> >
> >
> and just to add that we have a file specifically for /proc as it used
> to cause problems in early versions.
> 
> -Alan

Re: Files.read/readAllBytes can loop once with zero size buffer

[Alan Bateman]

On 29/07/2016 19:36, Martin Buchholz wrote:


I think keeping the status quo is the right outcome.  The /proc filesystem
is intentionally providing incorrect information because providing correct
information is deemed to be too expensive (have to compute contents of file
for stat?).  Reading /proc is sufficiently expensive that the extra realloc
of an empty buffer is noise.  Be thankful that reading files in /proc works
at all!


and just to add that we have a file specifically for /proc as it used to 
cause problems in early versions.


-Alan

Re: RFR (JAXP): 8162598 XSLTC transformer swallows empty namespace declaration which is needed to undeclare default namespace

[Joe Wang]

Hi Christoph,

All tests passed. Please add a note to the test on what's expected, or 
some javadoc to the method checkNodeNS8162598.


Best,
Joe

On 7/29/16, 7:55 AM, Langer, Christoph wrote:


Hi Joe,

here is the webrev after merging: 
http://cr.openjdk.java.net/~clanger/webrevs/8162598.2/ 



Let me know when you have done your tests -- then I'll push it.

@Daniel: Thanks for your help regarding Stack. You were right, I 
could remove that "".


Thanks & Best regards

Christoph

*From:*huizhe wang [mailto:huizhe.w...@oracle.com]
*Sent:* Freitag, 29. Juli 2016 08:04
*To:* Langer, Christoph ; Daniel Fuchs 


*Cc:* core-libs-dev@openjdk.java.net
*Subject:* Re: RFR (JAXP): 8162598 XSLTC transformer swallows empty 
namespace declaration which is needed to undeclare default namespace


Hi Christoph,

On 7/28/2016 6:10 AM, Langer, Christoph wrote:

Hi,

please review my change for the XSLT namespace issue.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8162598.1/


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

The issue has already been discussed in this thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-July/042525.html

Apart from the real fix in LiteralElement.java, method
translate(), I've done some further cleanups in a few places.
@Joe: The cleanups collide with some places of your proposed
change for https://bugs.openjdk.java.net/browse/JDK-8158084 where
we'd like to do the same things. So we'll have to synchronize on
pushing.


My patch has just pushed. Could you merge the changes and re-generate 
webrev?  The change looks to be sensitive. I'll build and run all 
other tests for you.


Best,
Joe


I've also enhanced the test case "TransformerTest" and added a
method which does a regression test for the bug reported.

Thanks in advance for reviewing.

Best regards

Christoph

Re: java.io.Writer uses CharSequence.toString()

[Bernd Eckenfels]

Hello,

yes I agree that has to be benchmarked. (And probably greatly varries
with the input length as well).


But besides the performance aspect I wanted to mention something else.
I had a password hashing API and wanted to switch from the typical
overwriteable char[] signature to a CharSequence. Because if the
password is only existing as a String there is no help in copying it to
a char[]. And depending on the source CharSequence can deal with all of
them. But then I noticed its quite hard to deal with CharSequences
where no intermediate Strings are constructed.

You asked about the "extract", I was inspired by Peters hint to
extractChar in StringUtil of OpenHFT. It is a scary magic in an
external API but it could be a internal shortcut within the JDK. I
would actally expect encoders to have the same shortcut for strings...

https://github.com/OpenHFT/Chronicle-Core/blob/master/src/main/java/net/openhft/chronicle/core/util/StringUtils.java

Gruss
Bernd


 Am Fri, 29 Jul 2016 18:21:37 +0100
schrieb Pavel Rappo :

> Once again, while I agree in some places it could have been done a
> bit better probably, I would say it's good to a have a look at
> benchmarks first.
> 
> If they show there's indeed a big difference between
> 
>char[] copy = new chars[charSequence.length()];
>String s = charSequence.toString();
>s.getChars(0, s.length, copy, 0);
> 
> and
> 
>char[] copy = new chars[charSequence.length()];
>charSequence.getChars(0, charSequence.length(), copy, 0);
> 
> it could justify an increase in complexity of CharBuffer.append or
> introducing a new default method (getChars/fillInto) into
> CharSequence. Possibly. Or maybe not. Because there might be some
> nontrivial effects we are completely unaware of.
> 
> Btw, what do you mean by "extract char[]" from StringBuilder? Do you
> want StringBuilder to give away a reference to its char[] outside? If
> not, than what's the difference between "extract char[]" from
> StringBuilder and "use String" in your algorithm?
> 
> The bottom line is whatever you suggest would likely need a good
> justification. To me it's not immediately obvious that something like
> this
> 
> public CharBuffer append(CharSequence csq) {
> if (csq == null) {
> put("null");
> } else if (csq instanceof StringBuilder) {
> char[] chars = new char[csq.length()];
> ((StringBuilder) csq).getChars(0, csq.length(), chars, 0);
> put(chars);
> } else if (csq instanceof StringBuffer) {
> char[] chars = new char[csq.length()];
> ((StringBuffer) csq).getChars(0, csq.length(), chars, 0);
> put(chars);
> } else if (csq instanceof CharBuffer) {
> CharBuffer buffer = (CharBuffer) csq;
> int p = buffer.position();
> put(buffer);
> buffer.position(p);
> } else {
> for (int i = 0; i < csq.length(); i++) {
> put(csq.charAt(i));
> }
> }
> return this;
> }
> 
> is better than this (what's there today)
> 
> public CharBuffer append(CharSequence csq) {
> if (csq == null)
> return put("null");
> else
> return put(csq.toString());
> }
> 
> > On 29 Jul 2016, at 15:12, e...@zusammenkunft.net wrote:
> > 
> > Hello,
> > 
> > Have to agree with Fabian handling CharSequences (and special case
> > StringBuilder) is pretty weak, in CharBuffer.append(CharSequence)
> > you see the same toString. I would expect it to do:
> > - Instamceof String -> use it
> > - Instance of StringBuilder -> extract char[] and iterate
> > - Instance of CharBuffer -> handle
> > - Otherwise: Loop over charAt
> > 
> > (the otherwise might be a tradeof between allocation and
> > (not)inlined bounds checks) 
> > 
> > Alternative would be a CharSequence.fillInto(char[])
> > 
> > BTW wouldn't it be create if char[] implements CharSequence?
> > 
> > Gruss
> > Bernd
> > -- 
> > http://bernd.eckenfels.net
> > From Win 10 Mobile
> > 
> > Von: Fabian Lange
> 

Re: [8u] request for approval: "8152172: PPC64: Support AES intrinsics"

[Volker Simonis]

Ping!

Can I please have an approval for backporting this change to 8u-dev?

Thanks,
Volker


On Fri, Jul 22, 2016 at 11:08 AM, Volker Simonis
 wrote:
> Hi,
>
> could you please approve the backport of the following, mostly ppc64
> change to jdk8u-dev:
>
> 8152172: PPC64: Support AES intrinsics
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8152172
> Webrev: http://cr.openjdk.java.net/~simonis/webrevs/2016/8152172_8u_hs/
> Webrev http://cr.openjdk.java.net/~simonis/webrevs/2016/8152172_8u_jdk/
> Review: 
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2016-March/thread.html#22032
> URL:http://hg.openjdk.java.net/jdk9/hs-comp/jdk/rev/74bc7be0777b
> URL:http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/68394bf0a09f
>
> As you can see, the change consists of two parts - the main one in the
> hotpsot repo and a small part in the jdk repo.
>
> The jdk part applied cleanly to jdk8u (with the usual directory shuffling).
>
> The hotspot part required a small tweak, but only in the ppc-only
> part. This is because the feature detection for the AES instructions
> was already active in jdk9 when the original change was made but is
> not available in jdk8u until now. You can find the additional changes
> at the end of this mail for your convenience.
>
> The required shared changes cleanly apply to jdk8u.
>
> As Vladimir pointed out in a previous thread, shared Hotspot changes
> have to go through JPRT even in jdk8u-dev. So I need a sponsor to do
> that and synchronously push both parts.
>
> @Hiroshii: could you please verify that you are fine with the change?
> I've done some tests on Power8LE but just want to be sure this
> downport satisfies your needs as well.
>
> Thank you and best regards,
> Volker
>
> =
>
> diff -r 15928d255046 src/cpu/ppc/vm/vm_version_ppc.cpp
> --- a/src/cpu/ppc/vm/vm_version_ppc.cpp Wed Jul 13 00:47:40 2016 -0700
> +++ b/src/cpu/ppc/vm/vm_version_ppc.cpp Fri Jul 22 10:32:36 2016 +0200
> @@ -452,6 +476,7 @@
>a->popcntw(R7, R5);  // code[7] -> popcntw
>a->fcfids(F3, F4);   // code[8] -> fcfids
>a->vand(VR0, VR0, VR0);  // code[9] -> vand
> +  a->vcipher(VR0, VR1, VR2);   // code[10] -> vcipher
>a->blr();
>
>// Emit function to set one cache line to zero. Emit function
> descriptor and get pointer to it.
> @@ -495,6 +520,7 @@
>if (code[feature_cntr++]) features |= popcntw_m;
>if (code[feature_cntr++]) features |= fcfids_m;
>if (code[feature_cntr++]) features |= vand_m;
> +  if (code[feature_cntr++]) features |= vcipher_m;
>
>// Print the detection code.
>if (PrintAssembly) {
> diff -r 15928d255046 src/cpu/ppc/vm/vm_version_ppc.hpp
> --- a/src/cpu/ppc/vm/vm_version_ppc.hpp Wed Jul 13 00:47:40 2016 -0700
> +++ b/src/cpu/ppc/vm/vm_version_ppc.hpp Fri Jul 22 10:32:36 2016 +0200
> @@ -42,6 +42,7 @@
>  fcfids,
>  vand,
>  dcba,
> +vcipher,
>  num_features // last entry to count features
>};
>enum Feature_Flag_Set {
> @@ -56,6 +57,7 @@
>  fcfids_m  = (1 << fcfids ),
>  vand_m= (1 << vand   ),
>  dcba_m= (1 << dcba   ),
> +vcipher_m = (1 << vcipher),
>  all_features_m= -1
>};
>static int  _features;
> @@ -83,6 +85,7 @@
>static bool has_fcfids()  { return (_features & fcfids_m) != 0; }
>static bool has_vand(){ return (_features & vand_m) != 0; }
>static bool has_dcba(){ return (_features & dcba_m) != 0; }
> +  static bool has_vcipher() { return (_features & vcipher_m) != 0; }
>
>static const char* cpu_features() { return _features_str; }

Re: RFR(S): 8162727 - Testbug: additional requires properties can't be used for filtering server vm in jdk jtreg tests

[Dmitrij Pochepko]

Sure,

now including core-libs-dev

Thanks,
Dmitrij

On 29.07.2016 20:16, joe darcy wrote:

Hello,

I also recommend sending this review request over to core-libs-dev 
since it affect the jdk repo.


Thanks,

-Joe


On 7/29/2016 12:35 AM, Dmitry Fazunenko wrote:

Hi Dmitrij,

The change itself looks good.

One note: this change adds a little overhead (a very little) for 
running tests in jdk repository, but it's required only for VM 
specific tests stored in jdk. As alternative of this change I can 
suggest moving VM specific tests from the 'jdk' to 'hotspot' 
repository. Perhaps, such massive update is too late for JDK9 time 
frame and could be done only in JDK10.
So, if the changes depending on 8162727 are not so critical and could 
be deferred I would prefer to postpone this fix.


Thanks,
Dima


On 29.07.2016 4:29, Vladimir Kozlov wrote:

It affects all groups. Should be reviewed on hotspot-dev.

Thanks,
Vladimir

On 7/28/16 10:19 AM, Dmitrij Pochepko wrote:

Hi,

please review small patch for 8162727 - Testbug: additional 
requires properties can't be used for filtering server vm in jdk 
jtreg tests


A problem is that tests under jdk repo can't use additional jtreg 
requires properties(like vm.flavor to filter out client or server 
vm). The same functionality was introduced for hotspot repo as part
of JDK-8154956. So, in order to filter tests using these additional 
properties a small fix is created.


webrev: http://cr.openjdk.java.net/~dpochepk/8162727/webrev.01/
CR: https://bugs.openjdk.java.net/browse/JDK-8162727

Thanks,
Dmitrij

Re: Files.read/readAllBytes can loop once with zero size buffer

[Martin Buchholz]

I think keeping the status quo is the right outcome.  The /proc filesystem
is intentionally providing incorrect information because providing correct
information is deemed to be too expensive (have to compute contents of file
for stat?).  Reading /proc is sufficiently expensive that the extra realloc
of an empty buffer is noise.  Be thankful that reading files in /proc works
at all!

On Thu, Jul 28, 2016 at 11:28 PM, Fabian Lange 
wrote:

> Hi,
> yes something like that. I have no strong opinion. After discovering
> this, I switched to a hand rolled implementation with file name
> depended estimated buffer sizes.
> But usually inside the JDK, Edge cases are handled better, usually
> with some Math.min()/Math.max logic.
>
> IMHO that really depends how common it could be that
> SeekableBytesChannel returns a size of 0 while actually containing
> data.
> If the proc files are the only realistic scenario, then maybe my
> concern is not that valid
>
> Fabian
>
> On Fri, Jul 29, 2016 at 8:21 AM,   wrote:
> > Hello,
> >
> >
> >
> > It could always read with a initial buffer of 0.5-16k and return a
> truncated
> > copy if it read less (and a omit truncation by returning shared static 0
> > length array if empty). But this will only optimize the 0 byte case.
> >
> >
> >
> >
> > Gruss
> > Bernd
> > --
> > http://bernd.eckenfels.net
> > From Win 10 Mobile
> >
> >
> >
> > Von: Martin Buchholz
> > Gesendet: Freitag, 29. Juli 2016 03:33
> > An: Fabian Lange
> > Cc: core-libs-dev
> > Betreff: Re: Files.read/readAllBytes can loop once with zero size buffer
> >
> >
> >
> > What do you suggest instead?
> >
> >
> >
> > The extra allocation of a zero-size buffer is not that bad.
> >
> >
> >
> > I think it's best to optimize for files with correct metadata, while
> >
> > tolerating ones with buggy metadata.
> >
> >
> >
> > On Thu, Jul 28, 2016 at 1:13 PM, Fabian Lange 
> >
> > wrote:
> >
> >
> >
> >> Hi,
> >
> >> I just noticed that when one uses Files.readAllBytes() to read for
> >
> >> example from the Linux proc file system, where file size is claimed to
> >
> >> be 0, Files.read() will allocate a buffer of size 0 before then
> >
> >> adjusting in a second loop (arraycopying the zero size buffer)
> >
> >>
> >
> >> In my opinion an initial size of 0 should not be used.
> >
> >> I admit this might be a regression for files which are really empty
> >
> >> and return an empty byte array, but I think it is actually more common
> >
> >> to read from files which incorrectly report to be zero sized.
> >
> >>
> >
> >> Fabian
> >
> >>
> >
> >
>

Re: java.io.Writer uses CharSequence.toString()

[Pavel Rappo]

Once again, while I agree in some places it could have been done a bit better
probably, I would say it's good to a have a look at benchmarks first.

If they show there's indeed a big difference between

   char[] copy = new chars[charSequence.length()];
   String s = charSequence.toString();
   s.getChars(0, s.length, copy, 0);

and

   char[] copy = new chars[charSequence.length()];
   charSequence.getChars(0, charSequence.length(), copy, 0);

it could justify an increase in complexity of CharBuffer.append or introducing a
new default method (getChars/fillInto) into CharSequence. Possibly. Or maybe
not. Because there might be some nontrivial effects we are completely unaware 
of.

Btw, what do you mean by "extract char[]" from StringBuilder? Do you want
StringBuilder to give away a reference to its char[] outside? If not, than
what's the difference between "extract char[]" from StringBuilder and "use
String" in your algorithm?

The bottom line is whatever you suggest would likely need a good justification.
To me it's not immediately obvious that something like this

public CharBuffer append(CharSequence csq) {
if (csq == null) {
put("null");
} else if (csq instanceof StringBuilder) {
char[] chars = new char[csq.length()];
((StringBuilder) csq).getChars(0, csq.length(), chars, 0);
put(chars);
} else if (csq instanceof StringBuffer) {
char[] chars = new char[csq.length()];
((StringBuffer) csq).getChars(0, csq.length(), chars, 0);
put(chars);
} else if (csq instanceof CharBuffer) {
CharBuffer buffer = (CharBuffer) csq;
int p = buffer.position();
put(buffer);
buffer.position(p);
} else {
for (int i = 0; i < csq.length(); i++) {
put(csq.charAt(i));
}
}
return this;
}

is better than this (what's there today)

public CharBuffer append(CharSequence csq) {
if (csq == null)
return put("null");
else
return put(csq.toString());
}

> On 29 Jul 2016, at 15:12, e...@zusammenkunft.net wrote:
> 
> Hello,
> 
> Have to agree with Fabian handling CharSequences (and special case 
> StringBuilder) is pretty weak, in CharBuffer.append(CharSequence) you see the 
> same toString. I would expect it to do:
> - Instamceof String -> use it
> - Instance of StringBuilder -> extract char[] and iterate
> - Instance of CharBuffer -> handle
> - Otherwise: Loop over charAt
> 
> (the otherwise might be a tradeof between allocation and (not)inlined bounds 
> checks) 
> 
> Alternative would be a CharSequence.fillInto(char[])
> 
> BTW wouldn't it be create if char[] implements CharSequence?
> 
> Gruss
> Bernd
> -- 
> http://bernd.eckenfels.net
> From Win 10 Mobile
> 
> Von: Fabian Lange

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

[Leela Mohan]

I think, change in the file unsafe.cpp is incorrect. (
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/ )

Below function is accessing naked oops when thread has transitioned to
"native":

*+ static jobject get_class_loader(JNIEnv* env, jclass cls) {**+   if
(java_lang_Class::is_primitive(JNIHandles::resolve_non_null(cls)))
{**+ return NULL;**+   }**+   Klass* k =
java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));**+   oop
loader = k->class_loader();**+   return JNIHandles::make_local(env,
loader);**+ }*


- Leela

On Mon, Sep 8, 2014 at 7:05 PM, Coleen Phillimore <
coleen.phillim...@oracle.com> wrote:

>
> Thanks, Mandy!
>
> Coleen
>
>
> On 9/8/14, 6:59 PM, Mandy Chung wrote:
>
>> Thumbs up.
>>
>> Mandy
>>
>> On 9/5/2014 12:55 PM, Coleen Phillimore wrote:
>>
>>> Summary: Add classLoader to java/lang/Class instance for fast access
>>>
>>> This is a backport request for 8u40.   This change has been in the jdk9
>>> code for 3 months without any problems.
>>>
>>> The JDK changes hg imported cleanly.  The Hotspot change needed a hand
>>> merge for create_mirror call in klass.cpp.
>>>
>>> http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
>>> http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/
>>>
>>> bug link https://bugs.openjdk.java.net/browse/JDK-6642881
>>>
>>> Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran
>>> jck java_lang tests with only the hotspot change.  The hotspot change can
>>> be tested separately from the jdk change (but not the other way around).
>>>
>>> Thanks,
>>> Coleen
>>>
>>
>>
>

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

[Leela Mohan]

Hi David,

I understand, Klass types are no longer oops  but
JNIHandles::resolve_non_null()
would expose naked oops. In other words, KlassOops are no longer oops but
java.lang.Class objects are.

Thanks,
Leela

On Thu, Jul 28, 2016 at 10:51 PM, David Holmes 
wrote:

> Hi Leela,
>
> On 29/07/2016 12:59 PM, Leela Mohan wrote:
>
>> I think, change in the file unsafe.cpp is incorrect. (
>> http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/ )
>>
>> Below function is accessing naked oops when thread has transitioned to
>> "native":
>>
>> *+ static jobject get_class_loader(JNIEnv* env, jclass cls) {**+   if
>> (java_lang_Class::is_primitive(JNIHandles::resolve_non_null(cls)))
>> {**+ return NULL;**+   }**+   Klass* k =
>> java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));**+   oop
>> loader = k->class_loader();**+   return JNIHandles::make_local(env,
>> loader);**+ }*
>>
>
> klass types are no longer oops in the Java heap, but metaspace objects
> that reside in a per-classloader allocation region. They never get
> compacted or relocated so raw pointers to them are safe.
>
> Cheers,
> David
>
>
>
>> - Leela
>>
>> On Mon, Sep 8, 2014 at 7:05 PM, Coleen Phillimore <
>> coleen.phillim...@oracle.com> wrote:
>>
>>
>>> Thanks, Mandy!
>>>
>>> Coleen
>>>
>>>
>>> On 9/8/14, 6:59 PM, Mandy Chung wrote:
>>>
>>> Thumbs up.

 Mandy

 On 9/5/2014 12:55 PM, Coleen Phillimore wrote:

 Summary: Add classLoader to java/lang/Class instance for fast access
>
> This is a backport request for 8u40.   This change has been in the jdk9
> code for 3 months without any problems.
>
> The JDK changes hg imported cleanly.  The Hotspot change needed a hand
> merge for create_mirror call in klass.cpp.
>
> http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
> http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/
>
> bug link https://bugs.openjdk.java.net/browse/JDK-6642881
>
> Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran
> jck java_lang tests with only the hotspot change.  The hotspot change
> can
> be tested separately from the jdk change (but not the other way
> around).
>
> Thanks,
> Coleen
>
>


>>>

RE: RFR (JAXP): 8162598 XSLTC transformer swallows empty namespace declaration which is needed to undeclare default namespace

[Langer, Christoph]

Hi Joe,

here is the webrev after merging: 
http://cr.openjdk.java.net/~clanger/webrevs/8162598.2/

Let me know when you have done your tests - then I'll push it.

@Daniel: Thanks for your help regarding Stack. You were right, I could 
remove that "".

Thanks & Best regards
Christoph


From: huizhe wang [mailto:huizhe.w...@oracle.com]
Sent: Freitag, 29. Juli 2016 08:04
To: Langer, Christoph ; Daniel Fuchs 

Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR (JAXP): 8162598 XSLTC transformer swallows empty namespace 
declaration which is needed to undeclare default namespace

Hi Christoph,
On 7/28/2016 6:10 AM, Langer, Christoph wrote:
Hi,

please review my change for the XSLT namespace issue.

Webrev: 
http://cr.openjdk.java.net/~clanger/webrevs/8162598.1/
Bug: https://bugs.openjdk.java.net/browse/JDK-8162598

The issue has already been discussed in this thread: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-July/042525.html

Apart from the real fix in LiteralElement.java, method translate(), I've done 
some further cleanups in a few places. @Joe: The cleanups collide with some 
places of your proposed change for 
https://bugs.openjdk.java.net/browse/JDK-8158084 where we'd like to do the same 
things. So we'll have to synchronize on pushing.

My patch has just pushed. Could you merge the changes and re-generate webrev?  
The change looks to be sensitive. I'll build and run all other tests for you.

Best,
Joe



I've also enhanced the test case "TransformerTest" and added a method which 
does a regression test for the bug reported.

Thanks in advance for reviewing.

Best regards
Christoph

AW: java.io.Writer uses CharSequence.toString()

[ecki]

Hello,

Have to agree with Fabian handling CharSequences (and special case 
StringBuilder) is pretty weak, in CharBuffer.append(CharSequence) you see the 
same toString. I would expect it to do:
- Instamceof String -> use it
- Instance of StringBuilder -> extract char[] and iterate
- Instance of CharBuffer -> handle
- Otherwise: Loop over charAt

(the otherwise might be a tradeof between allocation and (not)inlined bounds 
checks) 

Alternative would be a CharSequence.fillInto(char[])

BTW wouldn't it be create if char[] implements CharSequence?

Gruss
Bernd
-- 
http://bernd.eckenfels.net
>From Win 10 Mobile

Von: Fabian Lange

Re: RFR: 6543126: Level.known can leak memory

[Chris Hegarty]

On 29/07/16 12:54, Daniel Fuchs wrote:

Hi,

Here is the new webrev with Chris & James feedback
taken in.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/


This looks good to me Daniel.

-Chris.


best regards,

-- daniel

On 28/07/16 22:55, Daniel Fuchs wrote:

On 28/07/16 21:31, James Perkins wrote:

I just happened to take a glance at this and noticed the remove method
on the KnownLevels doesn't quite look right.
Using Optional.of(nameToLevels.get(mirroredLevel.name)).ifPresent((x) ->
x.remove(this)); will produce an NPE if the level is not present. If the
intention is that the levels may not be present in the list the
Optional.ofNullable() should be used.



Thanks a lot for your keen eyes James!

(sigh!) Why would Optional.of()  throw a NPE for null?
But it does...

This wasn't caught by the test because remove() usually
follows a purge() and every call to purge() is synchronized,
so I don't think it could happen, technically - except possibly
for the call purge(KnownLevel). But you're right and the code
is wrong, of course.

ofNullable() it is!

best regards,

-- daniel

Re: RFR: 6543126: Level.known can leak memory

[Daniel Fuchs]

Hi,

Here is the new webrev with Chris & James feedback
taken in.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/

best regards,

-- daniel

On 28/07/16 22:55, Daniel Fuchs wrote:

On 28/07/16 21:31, James Perkins wrote:

I just happened to take a glance at this and noticed the remove method
on the KnownLevels doesn't quite look right.
Using Optional.of(nameToLevels.get(mirroredLevel.name)).ifPresent((x) ->
x.remove(this)); will produce an NPE if the level is not present. If the
intention is that the levels may not be present in the list the
Optional.ofNullable() should be used.



Thanks a lot for your keen eyes James!

(sigh!) Why would Optional.of()  throw a NPE for null?
But it does...

This wasn't caught by the test because remove() usually
follows a purge() and every call to purge() is synchronized,
so I don't think it could happen, technically - except possibly
for the call purge(KnownLevel). But you're right and the code
is wrong, of course.

ofNullable() it is!

best regards,

-- daniel

Re: java.io.Writer uses CharSequence.toString()

[Pavel Rappo]

> On 29 Jul 2016, at 09:23, Fabian Lange  wrote:
> 
> This is especially sad because after having mad the String, write()
> will then copy out the chars of the string and then iterate over the
> chars individually.

As far as I can see no one iterates over chars individually. Instead, bulk write
is invoked with the char array from the String [1].

In contrast with java.io.OutputStream, java.io.Writer encourages bulk writes,
not single char writes. So the premise is that a concrete subclass provides
efficient bulk write. If this doesn't hold, then one could override 

Writer append(CharSequence csq)

in this class to behave exactly like you said.

> Something an iteration over chars of the CharSequence could have
> achieved much better.

I'm not sure about this. But you could prove it with a set of benchmarks for
concrete subclasses in the JDK.


[1] 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/9410dfad9f32/src/java.base/share/classes/java/io/Writer.java#l203

java.io.Writer uses CharSequence.toString()

[Fabian Lange]

Hi,
this week I learned that java.io.Writer while having
append(CharSequence) methods, will invoke toString() on them before
actually appending.
This is especially sad because after having mad the String, write()
will then copy out the chars of the string and then iterate over the
chars individually.

Something an iteration over chars of the CharSequence could have
achieved much better.

Fabian

Re: RFR: 8162439: Runtime.Version.parse needs fast-path for major versions

[Paul Sandoz]

> On 29 Jul 2016, at 05:15, Claes Redestad  wrote:
> 
> Hi Paul,
> 
> On 07/28/2016 02:55 PM, Paul Sandoz wrote:
>>> On 27 Jul 2016, at 19:36, Claes Redestad  wrote:
>>> 
>>> On 07/25/2016 08:01 PM, Iris Clark wrote:
 Hi, Claes.
 
> Webrev: http://cr.openjdk.java.net/~redestad/8162439/webrev.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8162439
 I think that this change looks good.  We provide a shortcut for the common 
 case where only the major version number is of interest without 
 introducing a new API.
>>> Thanks! Any reviewer want to give this the go-ahead?
>> Looks ok.
>> 
>> I suppose you could do:
>> 
>>   boolean isSimpleNumber(String s) {
>> for (int i = 0; i < s.length(); i++) {
>>   char c = s.get(i);
>>   if (c < ((i > 0) ? ‘0’ : ‘1’) || c > ‘9’)
>>   return false;
>> }
>> return true;
>>   }
>> 
>> 
>>   if (isSimpleNumber(s)) {
>> ...
>>   } else {
>> return VersionBuilder.parse(s);
>>   }
>> 
>> then let VersionBuilder.parse throw errors in incorrectly formatted version 
>> strings.
> 
> sounds reasonable. It'd still change the behavior for the empty string from 
> IAE to NFE, but only having to do one pass over the input string is nice.
> 

Hmm… i am inclined to agree because i am not sure given the pattern matching 
that one can reliably produce an NFE as anticipated in the JavaDoc.


> Another reasonable comment I got offline was that this patch splits the 
> parsing into two places, which is hacky. Since what we really want to avoid 
> is to eagerly load the Version string patterns (pulling in large parts of 
> j.u.regex), we could inline the parsing code into Runtime.Version.parse, 
> rename VersionBuilder to VersionPattern and access the constants therein from 
> the parse method to allow lazy initialization. The overall result is arguably 
> cleaner:
> 
> http://cr.openjdk.java.net/~redestad/8162439/webrev.02/
> 

Yes, much better. I was thinking similar thoughts about the split after i sent 
my previous email.

Paul.

Re: Files.read/readAllBytes can loop once with zero size buffer

[Fabian Lange]

Hi,
yes something like that. I have no strong opinion. After discovering
this, I switched to a hand rolled implementation with file name
depended estimated buffer sizes.
But usually inside the JDK, Edge cases are handled better, usually
with some Math.min()/Math.max logic.

IMHO that really depends how common it could be that
SeekableBytesChannel returns a size of 0 while actually containing
data.
If the proc files are the only realistic scenario, then maybe my
concern is not that valid

Fabian

On Fri, Jul 29, 2016 at 8:21 AM,   wrote:
> Hello,
>
>
>
> It could always read with a initial buffer of 0.5-16k and return a truncated
> copy if it read less (and a omit truncation by returning shared static 0
> length array if empty). But this will only optimize the 0 byte case.
>
>
>
>
> Gruss
> Bernd
> --
> http://bernd.eckenfels.net
> From Win 10 Mobile
>
>
>
> Von: Martin Buchholz
> Gesendet: Freitag, 29. Juli 2016 03:33
> An: Fabian Lange
> Cc: core-libs-dev
> Betreff: Re: Files.read/readAllBytes can loop once with zero size buffer
>
>
>
> What do you suggest instead?
>
>
>
> The extra allocation of a zero-size buffer is not that bad.
>
>
>
> I think it's best to optimize for files with correct metadata, while
>
> tolerating ones with buggy metadata.
>
>
>
> On Thu, Jul 28, 2016 at 1:13 PM, Fabian Lange 
>
> wrote:
>
>
>
>> Hi,
>
>> I just noticed that when one uses Files.readAllBytes() to read for
>
>> example from the Linux proc file system, where file size is claimed to
>
>> be 0, Files.read() will allocate a buffer of size 0 before then
>
>> adjusting in a second loop (arraycopying the zero size buffer)
>
>>
>
>> In my opinion an initial size of 0 should not be used.
>
>> I admit this might be a regression for files which are really empty
>
>> and return an empty byte array, but I think it is actually more common
>
>> to read from files which incorrectly report to be zero sized.
>
>>
>
>> Fabian
>
>>
>
>

AW: Files.read/readAllBytes can loop once with zero size buffer

[ecki]

Hello,

It could always read with a initial buffer of 0.5-16k and return a truncated 
copy if it read less (and a omit truncation by returning shared static 0 length 
array if empty). But this will only optimize the 0 byte case.


Gruss
Bernd
-- 
http://bernd.eckenfels.net
>From Win 10 Mobile

Von: Martin Buchholz

Re: RFR (JAXP): 8162598 XSLTC transformer swallows empty namespace declaration which is needed to undeclare default namespace

[huizhe wang]

Hi Christoph,

On 7/28/2016 6:10 AM, Langer, Christoph wrote:


Hi,

please review my change for the XSLT namespace issue.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8162598.1/ 



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



The issue has already been discussed in this thread: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-July/042525.html


Apart from the real fix in LiteralElement.java, method translate(), 
I’ve done some further cleanups in a few places. @Joe: The cleanups 
collide with some places of your proposed change for 
https://bugs.openjdk.java.net/browse/JDK-8158084 where we’d like to do 
the same things. So we’ll have to synchronize on pushing.




My patch has just pushed. Could you merge the changes and re-generate 
webrev?  The change looks to be sensitive. I'll build and run all other 
tests for you.


Best,
Joe

I’ve also enhanced the test case “TransformerTest” and added a method 
which does a regression test for the bug reported.


Thanks in advance for reviewing.

Best regards

Christoph