RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Peter, 

> -Original Message-
> From: Peter Levart 
> Sent: Freitag, 29. März 2019 16:44
> To: Lindenmaier, Goetz ; 'Mandy Chung'
> 
> Cc: core-libs-dev@openjdk.java.net; maurizio.cimadam...@oracle.com;
> hotspot-runtime-...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> 
> 
> On 3/29/19 4:36 PM, Peter Levart wrote:
> >
> >
> > On 3/29/19 8:49 AM, Lindenmaier, Goetz wrote:
> >> So I want to withdraw my claim that NPEs are thrown frequently.
> >> Probably I was biased by my compiler construction background,
> >> remembering NPE checks are all over the place in the code.
> >>
> >> But I think I can still keep the claim that the message is
> >> printed rarely.
> >>
> >> I'll adapt the JEP saying something like this:
> >> "While exceptions are supposed to be thrown rarely, i.e., only
> >> In exceptional situations, most are swallowed without ever
> >> looking at the message. Thus, overhead in getMessage() will
> >> not fall into account."
> >
> > Is this really a realistic assumption? That NPE exceptions are mostly
> > swallowed in most programs despite the fact that swallowing exceptions
> > (and throwing them to control the flow) is an anti-pattern? Is
> > majority of code really so badly written? I would expect that most
> > programs contain an exception handler of some kind that at least logs
> > all unexpected exceptions.
> >
> > I think JDK should assume that NPEs are not frequent in most well
> > written programs. Because in well written programs all unexpected
> > exceptions are at least logged somewhere and this alone guarantees
> > that programs are eventually "fixed" to not throw them frequently...
> >
> > Regards, Peter
> 
> So I would say that there are two kinds of programs (which kind is in
> majority doesn't matter):
> 
> a - programs that throw and catch exceptions for exceptional situations
> only (i.e. non frequently) - they also print the exceptions' messages
> b - programs that throw and swallow exceptions frequently, but they
> mostly don't print their messages
> 
> In either case .getMessage() is not called frequently for kind (a) and
> hopefully also for kind (b).
> 
> Regards, Peter
Hi,

I agree with this, and my numbers show
that the message is not printed frequently in any case.

Best regards,
  Goetz.

Re: RFR: 8221477: Inject os/cpu-specific constants into Unsafe from JVM

[David Holmes]

Follow up ...

On 1/04/2019 2:27 pm, David Holmes wrote:

Hi Andrew,

On 29/03/2019 8:40 pm, Andrew Dinn wrote:

Hi David,

Thanks very much for reviewing this patch.

On 29/03/2019 01:25, David Holmes wrote:

This seems fine in general but I have a few queries on some details:

src/hotspot/share/classfile/javaClasses.hpp

 f(java_lang_Thread) \
+   f(jdk_internal_misc_UnsafeConstants) \
 f(java_lang_ThreadGroup) \

Is there a reason this needs to be shoved in there? Similarly with
src/hotspot/share/classfile/systemDictionary.hpp:

    do_klass(Thread_klass,
java_lang_Thread  ) \
+   do_klass(UnsafeConstants_klass,
jdk_internal_misc_UnsafeConstants ) \
 do_klass(ThreadGroup_klass,
java_lang_ThreadGroup ) \

?


I'm not sure what you are asking here. Are you talking about the
positioning of these entries? If so then the reason for choosing those
positions was because they match the position of the corresponding class
initialization calls in the file thread.cpp. As to that choice ... see
below.


Yes I'm asking about the positioning ...


src/hotspot/share/runtime/thread.cpp

 main_thread->set_threadObj(thread_object);
+
+   // initialize the hardware-specific constants needed by Unsafe
+   initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(),
CHECK);
+   jdk_internal_misc_UnsafeConstants::set_unsafe_constants();
+
 // Set thread status to running since main thread has
 // been started and running.
 java_lang_Thread::set_thread_status(thread_object,

That seems a very odd place to insert the new initialization. I can't
see any reason you'd need to split the Thread object initialization like
that. ??


Well, yes, indeed :-). I was not sure where this init needed to go so I
simply put it in as early as I thought was safe. Clearly, it is an
interloper into intitialise_java_lang_classes() but I was not sure where
else was more appropriate.

I don't think it matters too much where this init happens so long as it
is early enough to precede any clinit dependencies on Unsafe (see
below). I'm very happy to take advice on this (indeed I was hoping/
expecting it would be brought up at review) and also on where the
entries in the headers would best be placed.


More generally exactly when do you need to initialize this new class by
and how does the initialization order change before/after this fix? (I'm
expecting only to see the new class inserted where needed without any
other perturbations.)


As I said, I put the class initialization in at this early point simply
to guarantee that the constants are available before Unsafe or any class
which might recursively initialize Unsafe could end up needing them. I
am sure they could move later and also earlier, although the latter
would probably not make any sense. The important thing is that they
don't really have any hard dependencies on other class inits apart,
perhaps, from 'magic' classes like Object, Class etc which need to exist
in order to init /any/ other class.


I did some analysis of the class loading and initialization sequence and 
added my suggestions to bug report. In summary loading seems somewhat 
immaterial so I suggest:


- javaClasses.hpp: Add UnsafeConstants at the end of 
BASIC_JAVA_CLASSES_DO_PART2

- systemDictionary.hpp: Add UnsafeConstants immediately before Unsafe

Then for init:
- thread.cpp: initialize UnsafeConstants immediately after j.l.Module

It would be desirable to detect if we happen to execute the  
earlier (by accident) so I suggest adding a "not initialized" assertion 
prior to your code calling initialize_class. Actually that might be a 
useful addition to the initialize_class method, as if it fires it means 
we're not initializing in the expected order ... I'll run a little 
adding that ...


I meant to say "run a little test". Turns out you can't put the 
assertion in initialize_class as the initialization can vary for some of 
the exception classes (at least) depending on VM flags used (e.g. -Xrs, 
and I think certain logging options).


Thanks,
David



Thanks,
David

P.S. This missing space in javaClasses.cpp was reported by Thomas but 
hasn't been fixed yet:


4026 }else {



I deliberately factored these constants out of Unsafe into a separate
all static class UnsafeConstants so as to decouple this init from any
current or future dependencies on Unsafe (and also to make them more
clearly visible). Since the fields only hold os/hw specific constants
they can be set any time after VM_Version/CPU-specific init and
OS-specific init has completed.


src/hotspot/share/classfile/vmSymbols.hpp

+   template(big_endian_name,   "BIG_ENDIAN")
    \
+   template(use_unaligned_access_name,
"UNALIGNED_ACCESS")    \

Nit: There's an extra space before "UNALIGNED...


Thanks. Thomas Stuefe already spotted that and I have updated the webrev
in place

Re: [13] RFR: 8205432: Replace the placeholder Japanese era name

[Chris Hegarty]

On 01/04/2019 06:33, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issue:

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

The CSR and the proposed changeset are located at:

https://bugs.openjdk.java.net/browse/JDK-8218207
http://cr.openjdk.java.net/~naoto/8205432/webrev.03/


Looks good to me.

-Chris.

Re: [13]: RFR: 8174268: Declare a public field in JapaneseEra for the era starting May 2019

[Chris Hegarty]

On 01/04/2019 06:33, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issue:

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

The CSR and the proposed changeset are located at:

https://bugs.openjdk.java.net/browse/JDK-8193826
http://cr.openjdk.java.net/~naoto/8174268/webrev.02/


Looks good Naoto.

-Chris.

RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

Hi,

when a String has a calculated hash code value of 0, we recalculate and
store a 0 to the String.hash field every time (except for the empty
String, which is special cased). To make String objects more amenable to
storage in shared read-only memory, e.g., CDS archives, we should avoid
this redundant store.

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

Testing: tier1-3, no regression on existing and new StringHashCode
micros

/Claes

Re: RFR: 8221723: Avoid storing zero to String.hash

[Pavel Rappo]

Hi Claes,

> To make String objects more amenable to storage in shared read-only memory,
> e.g., CDS archives, we should avoid this redundant store.

Could you please elaborate on that?

Re: RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

Hi Pavel,

On 2019-04-01 14:20, Pavel Rappo wrote:

Hi Claes,


To make String objects more amenable to storage in shared read-only memory,
e.g., CDS archives, we should avoid this redundant store.


Could you please elaborate on that?



currently, heap archiving excludes Strings with hash 0 from being
archived. This includes "" etc. I intend to fix that in JDK-8221724[1]
but that'd then lead to read-only memory pages backed by the archive to
be dirtied when getting the hash code of such Strings (it wouldn't
break, but it'd be effectively copy-on-write and diminish the benefit
of storing Strings in archives and similar solutions). This patch merely
ensures that such dirtying doesn't happen and that we can archive
0-hashcode Strings without repercussions.

Makes sense?

/Claes

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

Re: RFR: 8221723: Avoid storing zero to String.hash

[Aleksey Shipilev]

On 4/1/19 1:57 PM, Claes Redestad wrote:
> when a String has a calculated hash code value of 0, we recalculate and
> store a 0 to the String.hash field every time (except for the empty
> String, which is special cased). To make String objects more amenable to
> storage in shared read-only memory, e.g., CDS archives, we should avoid
> this redundant store.
> 
> Bug:    https://bugs.openjdk.java.net/browse/JDK-8221723
> Webrev: http://cr.openjdk.java.net/~redestad/8221723/

Looks fine. I recall we were fixing something like this, but I might be 
confusing it with the other
RFE [0]. The comment might emphasize we are doing it for RO memory:

   // Avoid issuing a store if the calculated value is also zero:
   // in addition to minor optimization benefit, this also allows storing
   // Strings with zero hash code in read-only memory.

-Aleksey

[0] https://bugs.openjdk.java.net/browse/JDK-6921374

Re: RFR: 8221723: Avoid storing zero to String.hash

[Pavel Rappo]

> On 1 Apr 2019, at 13:36, Claes Redestad  wrote:
> 
> Makes sense?

It does, thanks. I wonder though what portion of strings in a typical app has a
calculated `hash` of 0? My naive estimate would be 1E-9. Unless I'm mistaken,
is that really of concern?

Re: RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

Hi Aleksey,

On 2019-04-01 15:03, Aleksey Shipilev wrote:

On 4/1/19 1:57 PM, Claes Redestad wrote:

when a String has a calculated hash code value of 0, we recalculate and
store a 0 to the String.hash field every time (except for the empty
String, which is special cased). To make String objects more amenable to
storage in shared read-only memory, e.g., CDS archives, we should avoid
this redundant store.

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


Looks fine. I recall we were fixing something like this, but I might be 
confusing it with the other
RFE [0].


digging up the history I did a regression fix due some benchmarks
regressing during JDK 9:
http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/6837759aa403 (closed bug
due some clerical errors linking to internal systems..)

Cause of regression was https://bugs.openjdk.java.net/browse/JDK-8058643

The approach of avoiding the store for any String was however lost when
the JEP work for Compact Strings was integrated, and since the
performance benefit for anything by "" is comparatively slim I didn't
insist on fixing this at the time. With the added motivation that it
helps storing Strings on read-only memory it feels worthwhile, though.


The comment might emphasize we are doing it for RO memory:

// Avoid issuing a store if the calculated value is also zero:
// in addition to minor optimization benefit, this also allows storing
// Strings with zero hash code in read-only memory.


Yes, that reads better and better emphasizes the point I'm trying to
make. Updated in place.

Thanks!

/Claes

Re: [13]: RFR: 8174268: Declare a public field in JapaneseEra for the era starting May 2019

[Roger Riggs]

Looks good.

On 04/01/2019 07:20 AM, Chris Hegarty wrote:


On 01/04/2019 06:33, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issue:

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

The CSR and the proposed changeset are located at:

https://bugs.openjdk.java.net/browse/JDK-8193826
http://cr.openjdk.java.net/~naoto/8174268/webrev.02/


Looks good Naoto.

-Chris.

Re: RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

On 2019-04-01 15:17, Pavel Rappo wrote:

On 1 Apr 2019, at 13:36, Claes Redestad  wrote:

Makes sense?


It does, thanks. I wonder though what portion of strings in a typical app has a
calculated `hash` of 0? My naive estimate would be 1E-9. Unless I'm mistaken,
is that really of concern?



Specifically I'm looking at an archiving RFE that is blocked by the fact
that interned empty String literals loses identity when archived and
reconstituted, and there exists code that does identity comparisons on
such interned empty String, e.g.:

http://hg.openjdk.java.net/jdk/jdk/file/d02f1f4ff3a6/src/java.base/share/classes/java/util/ResourceBundle.java#l3455

/Claes

Re: RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

On 2019-04-01 15:28, Claes Redestad wrote:

On 2019-04-01 15:17, Pavel Rappo wrote:
On 1 Apr 2019, at 13:36, Claes Redestad  
wrote:


Makes sense?


It does, thanks. I wonder though what portion of strings in a typical 
app has a
calculated `hash` of 0? My naive estimate would be 1E-9. Unless I'm 
mistaken,

is that really of concern?



Specifically I'm looking at an archiving RFE that is blocked by the fact
that interned empty String literals loses identity when archived and
reconstituted, and there exists code that does identity comparisons on
such interned empty String, e.g.:

http://hg.openjdk.java.net/jdk/jdk/file/d02f1f4ff3a6/src/java.base/share/classes/java/util/ResourceBundle.java#l3455 



RFE in question: https://bugs.openjdk.java.net/browse/JDK-8221701

Technically we could special-case the empty String, but I much prefer
a more general solution like this + JDK-8221724

/Claes

Re: [13] RFR: 8205432: Replace the placeholder Japanese era name

[Roger Riggs]

+1

On 04/01/2019 07:18 AM, Chris Hegarty wrote:

On 01/04/2019 06:33, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issue:

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

The CSR and the proposed changeset are located at:

https://bugs.openjdk.java.net/browse/JDK-8218207
http://cr.openjdk.java.net/~naoto/8205432/webrev.03/


Looks good to me.

-Chris.

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan,

thanks for getting back on this.

> One thing that is puzzling is that
> ZipFileAttributeView/ZipFileAttributes extend
> PosixFileAttributeView/PosixFileAttributes. I don't think that will work
> because the "zip" view is supported by default whereas "posix" view
> needs the file system to be created with enablePosixFileAttributes=true.

Hm, when I was looking at it initially, I was also wondering if it would be 
cleaner either have a default ZipFileAttributeView/ZipFileAttributes 
implementation that doesn't extend Posix or an "Enhanced" 
ZipFileAttributeView/ZipFileAttributes that would extend Posix. But I saw in 
the implementation of ZipFileAttributeView::get that this is already the "gate" 
where the requester would get the ZipFileAttributeView implementation with the 
requested behavior set. So I was hoping that it'd be fine to handle the Posix 
extension this way. Do you really think that wouldn't work?

Alternatively, I could explore a different class hierarchy for 
ZipFileAttributeView/ZipFileAttributes...

> I saw your comment about naming the file permissions attribute
> "storedPermissions" in the zip view but if the zip and posix view are
> separated then I think it would be clearer to name it "permissions" in
> the zip view. If code is using Files.getAttribute then it will need the
> view name so they won't get mixed up.

Let me think about it...

> You asked about the fallback when defaultOwner/defaultGroup aren't set.
> If getOwner fails with an IOException then I think that exception should
> be propagated. The UOE case will fallback to the value of "user.name". I
> think the fallback for the group owner should be the file owner rather
> than "
> "". The default.policy file will need to be updated to
> grant jdk.zipfs RuntimePermission("accessUserInfo") so that you won't
> need to deal with security exceptions.

Sounds fine. I'll implement that.

> As regards next steps then I think we need to agree the spec changes, as
> in the the javadoc in module-info.java. Once we have the spec agreed
> then the CSR can be updated and finalized. The code review can be done
> in parallel of course. I think Lance is going to help review the changes.

Ok, I guess this eventually boils down to agree upon the right way of doing the 
"permissions" attribute or is there something more related to the spec?

Thanks
Christoph

Re: Request for sponsor: JDK-8221430: StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings specified

[Roger Riggs]

Hi Ivan,

Thanks for running the micro benchmarks.

This version has more code duplication than Andrew's original
proposal that calculated the coder only CharSequence and had
a single AbstractStringBuilder constructor for computing the size
and allocating the byte[]/

http://cr.openjdk.java.net/~aleonard/8221430/webrev.00/

I'd be curious to know the JMH tests for that version compared.

Another comment is whether the 'instanceof' code is the
best performer for checking if the argument is a String.
I might think that 'seq.getClass().equals(String.class)' is faster.

And in this most recent webrev that has separated the AbstractStringBuilder
constructors for String from CharSequence, is it more likely that the 
argument
will be an AbstractStringBuilder than a String so that comparison should 
be done first.


Thanks, Roger

On 03/30/2019 02:29 AM, Ivan Gerasimov wrote:

It was a good suggestion to run micro benchmarks!

First, it turned out that (at least in some scenarios) 
StringBuilder(String) is not intrinsified.
I.e., running benchmarks with/without '-XX:+UnlockDiagnosticVMOptions 
-XX:DisableIntrinsic=_StringBuilder_String' led to the same numbers.


Here they are prior the fix:
Benchmark   Mode  Cnt   Score Error Units
StringBuilders.fromLatin1String avgt   18  16.378 ± 0.378 ns/op
StringBuilders.fromLatin1StringBuilder  avgt   18  19.975 ± 0.195 ns/op
StringBuilders.fromUtf8String   avgt   18  19.946 ± 2.774 ns/op
StringBuilders.fromUtf8StringBuilder    avgt   18  34.317 ± 0.415 ns/op

Second, with the fix from the webrev.01, the performance has degraded:
Benchmark   Mode  Cnt   Score Error Units
StringBuilders.fromLatin1String avgt   18  24.517 ± 0.965 ns/op
StringBuilders.fromLatin1StringBuilder  avgt   18  20.025 ± 0.678 ns/op
StringBuilders.fromUtf8String   avgt   18  26.102 ± 0.627 ns/op
StringBuilders.fromUtf8StringBuilder    avgt   18  23.141 ± 0.394 ns/op

So, it appears to be necessary to handle the case 
StringBuilder(String) separately from the general case.


And here there is the new webrev:
http://cr.openjdk.java.net/~igerasim/8221430/02/webrev/

This variant restores the performance, and shows some improvement in a 
case of StringBuilder(StringBuilder-with-UTF8-content):

Benchmark   Mode  Cnt   Score Error Units
StringBuilders.fromLatin1String avgt   18  15.742 ± 0.189 ns/op
StringBuilders.fromLatin1StringBuilder  avgt   18  18.671 ± 0.109 ns/op
StringBuilders.fromUtf8String   avgt   18  17.172 ± 0.100 ns/op
StringBuilders.fromUtf8StringBuilder    avgt   18  21.885 ± 0.138 ns/op

With kind regards,
Ivan


On 3/28/19 2:24 PM, Ivan Gerasimov wrote:

Apologize for the delay, got distracted by other urgent things.

I've got some surprising micro-benchmark results, and need to 
understand them before pushing the fix.


I'll verify the results and then share details.

With kind regards,
Ivan


On 3/28/19 9:41 AM, Roger Riggs wrote:

Hi Andrew,

I'm fine with Ivan's version too.

Roger


On 03/28/2019 12:13 PM, Andrew Leonard wrote:
Roger, Ivan, thanks for your help discussing this issue. Fyi, I am 
off on leave for the next week, so I +1 Ivan's last webrev if 
that's the way you decide to go..

Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com




From: Roger Riggs 
To: Ivan Gerasimov , Andrew Leonard 


Cc: core-libs-dev@openjdk.java.net
Date: 27/03/2019 13:39
Subject: Re: Request for sponsor: JDK-8221430: 
StringBuffer(CharSequence) constructor truncates when 
-XX:-CompactStrings specified
 





Hi Ivan,

On 03/26/2019 07:30 PM, Ivan Gerasimov wrote:
The main source of confusion here seems to be due to that the coder 
is passed in as an argument, so it either needs to be trusted or 
has to be verified in the constructor.


The design for coder is that it *is* trusted in all 
internal/implementation APIs.
Subsequent changes should not weaken this invariant, it will cause 
doubt

in the code and cast doubt on all of the performance work that has
been done to optimize its use.

So, let's instead introduce AbstractStringBuilder(CharSequence) and 
make it do all manipulations.

_
__http://cr.openjdk.java.net/~igerasim/8221430/01/webrev/_ 



Note that the common case of StringBuilder(String) already gets 
intrinsified by hotspot.


What do you think?

This looks good, the logic is in one place.

Since StringBuilder(String) is an intrinsic, my doubt about a 
slight performance

impact is unwarranted in that specific case.

Are there any StringBuffer/Builder jmh tests than be easily rerun 
to compare?


Thanks, Roger


With kind regards,
Ivan

On 3/26/19 1:04 PM, Ivan Gerasimov wrote:
From the design poin

RFR: 8221701: Archive constant BaseLocales

[Claes Redestad]

Hi,

by archiving constant BaseLocale objects, we can simplify (and speed up)
creation of the constant Locale objects in java.util.Locale which are
unconditionally created during startup.

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

Testing: tier1-3, along with patches for JDK-8221723 and
JDK-8221724

Performance: a small reduction in bytecode executed during startup (-6k,
or ~1% of bootstrap), a small reduction in number of classes loaded
on small applications (458 -> 451 on a simple Hello World).

Thanks!

/Claes

Re: RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

Hi Jiangli,

On 2019-04-01 17:21, Jiangli Zhou wrote:



On Mon, Apr 1, 2019 at 4:59 AM Claes Redestad > wrote:


Hi,

when a String has a calculated hash code value of 0, we recalculate and
store a 0 to the String.hash field every time (except for the empty
String, which is special cased). To make String objects more amenable to
storage in shared read-only memory, e.g., CDS archives, we should avoid
this redundant store.


That was exactly the reason why 0-hash string was excluded from the 
closed archive heap region.


Thanks for confirming!




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


Looks good to me.


Thanks!

/Claes

Re: [13] RFR: 8205432: Replace the placeholder Japanese era name

[Remi Forax]

Hi Naoto,
just for my education,
what is the translation of "Reiwa" ?

regards,
Rémi

- Mail original -
> De: "naoto sato" 
> À: i18n-...@openjdk.java.net, "core-libs-dev" 
> Envoyé: Lundi 1 Avril 2019 07:33:09
> Objet: [13] RFR: 8205432: Replace the placeholder Japanese era name

> Hello,
> 
> Please review the fix to the following issue:
> 
> https://bugs.openjdk.java.net/browse/JDK-8205432
> 
> The CSR and the proposed changeset are located at:
> 
> https://bugs.openjdk.java.net/browse/JDK-8218207
> http://cr.openjdk.java.net/~naoto/8205432/webrev.03/
> 
> The fix is simply to replace all the occurrences of the placeholder name
> "NewEra"/"元号" with "Reiwa"/"令和", which is the government declared era
> name.
> 
> Naoto

Re: [13] RFR: 8205432: Replace the placeholder Japanese era name

[Martin Buchholz]

https://en.wikipedia.org/wiki/Reiwa_period

On Mon, Apr 1, 2019 at 9:16 AM Remi Forax  wrote:

> Hi Naoto,
> just for my education,
> what is the translation of "Reiwa" ?
>

Re: RFR: 8221723: Avoid storing zero to String.hash

[Martin Buchholz]

I'm confused.

We have always had
 if (h == 0 && value.length > 0) {
so we have already been special-casing empty strings (maybe we should
stop?).

Java has guaranteed that string literals are unique strings, but the empty
string is not special in this regard.  Archiving any string literal and
restoring it later should be identity-preserving, no?

Whenever we test or benchmark zero-hash Strings, there are 3 cases that
should be covered - literal "", new String(), and (rare!) non-empty Strings
that happen to hash to 0.

We could avoid archiving those rare non-empty Strings that happen to hash
to 0 at archive time if it saved us a branch at runtime.

Re: RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

We could remove value.length > 0 and be net-neutral in #branches
for the general slow path (calculate the hash), but that would add a
branch to the emptyString.hashCode() _fast path_ (consistently add
~0.5ns/op on my workstation with provided micro). We've been bitten
before that adding cost to "".hashCode() has effect on some benchmarks,
e.g., our internal XML implementation has some odd corner cases.

Thus optimizing away a single branch from the calculating slow path (5%
for the empty string, quickly diminishing overhead with increased String
size) didn't seem worthwhile to me.

/Claes

On 2019-04-01 19:13, Martin Buchholz wrote:

I'm confused.

We have always had
          if (h == 0 && value.length > 0) {
so we have already been special-casing empty strings (maybe we should 
stop?).


Java has guaranteed that string literals are unique strings, but the 
empty string is not special in this regard.  Archiving any string 
literal and restoring it later should be identity-preserving, no?


Whenever we test or benchmark zero-hash Strings, there are 3 cases that 
should be covered - literal "", new String(), and (rare!) non-empty 
Strings that happen to hash to 0.


We could avoid archiving those rare non-empty Strings that happen to 
hash to 0 at archive time if it saved us a branch at runtime.

Re: RFR: 8221723: Avoid storing zero to String.hash

[Martin Buchholz]

Let me try again...  We exclude 0-hash Strings from archiving because we
might write to the hash field,
BUT the existing guard
value.length > 0
ensures that won't happen for empty Strings ... so WHY were we excluding
empty Strings from archiving?


On Mon, Apr 1, 2019 at 10:32 AM Claes Redestad 
wrote:

> We could remove value.length > 0 and be net-neutral in #branches
> for the general slow path (calculate the hash), but that would add a
> branch to the emptyString.hashCode() _fast path_ (consistently add
> ~0.5ns/op on my workstation with provided micro). We've been bitten
> before that adding cost to "".hashCode() has effect on some benchmarks,
> e.g., our internal XML implementation has some odd corner cases.
>
> Thus optimizing away a single branch from the calculating slow path (5%
> for the empty string, quickly diminishing overhead with increased String
> size) didn't seem worthwhile to me.
>
> /Claes
>
> On 2019-04-01 19:13, Martin Buchholz wrote:
> > I'm confused.
> >
> > We have always had
> >   if (h == 0 && value.length > 0) {
> > so we have already been special-casing empty strings (maybe we should
> > stop?).
> >
> > Java has guaranteed that string literals are unique strings, but the
> > empty string is not special in this regard.  Archiving any string
> > literal and restoring it later should be identity-preserving, no?
> >
> > Whenever we test or benchmark zero-hash Strings, there are 3 cases that
> > should be covered - literal "", new String(), and (rare!) non-empty
> > Strings that happen to hash to 0.
> >
> > We could avoid archiving those rare non-empty Strings that happen to
> > hash to 0 at archive time if it saved us a branch at runtime.
>

Re: RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

On 2019-04-01 20:01, Martin Buchholz wrote:
Let me try again...  We exclude 0-hash Strings from archiving because we 
might write to the hash field,

BUT the existing guard
value.length > 0
ensures that won't happen for empty Strings ... so WHY were we excluding 
empty Strings from archiving?


I don't think there was a strong reason for that, except maybe
unwillingness to convolute the VM code. I'm now trying to remove this
exclusion/limitation altogether with 8221724 alongside this RFE.

Thanks!

/Claes

Re: RFR: 8221723: Avoid storing zero to String.hash

[dean . long]

Wouldn't it be better to write a non-0 value when the computed hash code 
is 0, so we don't have to recompute it?  Is there some advantage to 
writing 0 instead of any other value, such as 1?


dl

On 4/1/19 4:57 AM, Claes Redestad wrote:

Hi,

when a String has a calculated hash code value of 0, we recalculate and
store a 0 to the String.hash field every time (except for the empty
String, which is special cased). To make String objects more amenable to
storage in shared read-only memory, e.g., CDS archives, we should avoid
this redundant store.

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

Testing: tier1-3, no regression on existing and new StringHashCode
micros

/Claes

Re: RFR: 8221723: Avoid storing zero to String.hash

[Martin Buchholz]

The spec says that "".hashCode() must be 0.
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#hashCode()


On Mon, Apr 1, 2019 at 12:51 PM  wrote:

> Wouldn't it be better to write a non-0 value when the computed hash code
> is 0, so we don't have to recompute it?  Is there some advantage to
> writing 0 instead of any other value, such as 1?
>
> dl
>
> On 4/1/19 4:57 AM, Claes Redestad wrote:
> > Hi,
> >
> > when a String has a calculated hash code value of 0, we recalculate and
> > store a 0 to the String.hash field every time (except for the empty
> > String, which is special cased). To make String objects more amenable to
> > storage in shared read-only memory, e.g., CDS archives, we should avoid
> > this redundant store.
> >
> > Bug:https://bugs.openjdk.java.net/browse/JDK-8221723
> > Webrev: http://cr.openjdk.java.net/~redestad/8221723/
> >
> > Testing: tier1-3, no regression on existing and new StringHashCode
> > micros
> >
> > /Claes
> >
>
>

Re: RFR: 8221723: Avoid storing zero to String.hash

[dean . long]

OK, I guess there's no ideal solution.  Adding a separate "not-computed" 
boolean adds space, and using a different sentinel value for 
"not-computed" would probably be slower on CPUs that have a fast 
compare-and-branch-against-zero instruction.


dl

On 4/1/19 12:55 PM, Martin Buchholz wrote:

The spec says that "".hashCode() must be 0.
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#hashCode()


On Mon, Apr 1, 2019 at 12:51 PM > wrote:


Wouldn't it be better to write a non-0 value when the computed
hash code
is 0, so we don't have to recompute it?  Is there some advantage to
writing 0 instead of any other value, such as 1?

dl

On 4/1/19 4:57 AM, Claes Redestad wrote:
> Hi,
>
> when a String has a calculated hash code value of 0, we
recalculate and
> store a 0 to the String.hash field every time (except for the empty
> String, which is special cased). To make String objects more
amenable to
> storage in shared read-only memory, e.g., CDS archives, we
should avoid
> this redundant store.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8221723
> Webrev: http://cr.openjdk.java.net/~redestad/8221723/
>
> Testing: tier1-3, no regression on existing and new StringHashCode
> micros
>
> /Claes
>

Re: RFR: 8221723: Avoid storing zero to String.hash

[Brian Goetz]

There's another reason to avoid these writes, besides CDS optimizations: 
do-nothing writes generate useless card mark activity.


On 4/1/2019 7:57 AM, Claes Redestad wrote:

Hi,

when a String has a calculated hash code value of 0, we recalculate and
store a 0 to the String.hash field every time (except for the empty
String, which is special cased). To make String objects more amenable to
storage in shared read-only memory, e.g., CDS archives, we should avoid
this redundant store.

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

Testing: tier1-3, no regression on existing and new StringHashCode
micros

/Claes

Re: RFR: 8221723: Avoid storing zero to String.hash

[Aleksey Shipilev]

On 4/1/19 10:08 PM, Brian Goetz wrote:
> There's another reason to avoid these writes, besides CDS optimizations: 
> do-nothing writes generate
> useless card mark activity.

Not for primitive stores, like hash code. But you can construct a funny 
workload where storing zero
hash code would false-share with something important. I think CDS is the major 
use case here, and it
is a legit one.

-Aleksey

Re: RFR: 8221723: Avoid storing zero to String.hash

[Claes Redestad]

We actually looked at this idea earlier today, and squeezing a "not-
computed" value into String might be "free" since there seems to be a
padding gap on all VM varieties (at least according to JOL estimates[1])

That'd be a larger endeavor, though, since there are places in VM that
calculates and injects the String.hash value.

/Claes

[1] https://openjdk.java.net/projects/code-tools/jol/

On 2019-04-01 22:02, dean.l...@oracle.com wrote:
OK, I guess there's no ideal solution.  Adding a separate "not-computed" 
boolean adds space, and using a different sentinel value for 
"not-computed" would probably be slower on CPUs that have a fast 
compare-and-branch-against-zero instruction.


dl

On 4/1/19 12:55 PM, Martin Buchholz wrote:

The spec says that "".hashCode() must be 0.
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#hashCode() 




On Mon, Apr 1, 2019 at 12:51 PM > wrote:


    Wouldn't it be better to write a non-0 value when the computed
    hash code
    is 0, so we don't have to recompute it?  Is there some advantage to
    writing 0 instead of any other value, such as 1?

    dl

    On 4/1/19 4:57 AM, Claes Redestad wrote:
    > Hi,
    >
    > when a String has a calculated hash code value of 0, we
    recalculate and
    > store a 0 to the String.hash field every time (except for the empty
    > String, which is special cased). To make String objects more
    amenable to
    > storage in shared read-only memory, e.g., CDS archives, we
    should avoid
    > this redundant store.
    >
    > Bug: https://bugs.openjdk.java.net/browse/JDK-8221723
    > Webrev: http://cr.openjdk.java.net/~redestad/8221723/
    >
    > Testing: tier1-3, no regression on existing and new StringHashCode
    > micros
    >
    > /Claes
    >

Re: RFR: 8221723: Avoid storing zero to String.hash

[Remi Forax]

- Mail original -
> De: "Claes Redestad" 
> À: "Dean Long" , "Martin Buchholz" 
> Cc: "core-libs-dev" 
> Envoyé: Lundi 1 Avril 2019 22:44:46
> Objet: Re: RFR: 8221723: Avoid storing zero to String.hash

> We actually looked at this idea earlier today, and squeezing a "not-
> computed" value into String might be "free" since there seems to be a
> padding gap on all VM varieties (at least according to JOL estimates[1])

Also the header of the underlying array (the array of bytes/chars) has space 
for a hashCode (the system one) but that value is never visible from the user 
POV,
so you can use one bit of it.

> 
> That'd be a larger endeavor, though, since there are places in VM that
> calculates and injects the String.hash value.
> 
> /Claes

Rémi

> 
> [1] https://openjdk.java.net/projects/code-tools/jol/



> 
> On 2019-04-01 22:02, dean.l...@oracle.com wrote:
>> OK, I guess there's no ideal solution.  Adding a separate "not-computed"
>> boolean adds space, and using a different sentinel value for
>> "not-computed" would probably be slower on CPUs that have a fast
>> compare-and-branch-against-zero instruction.
>> 
>> dl
>> 
>> On 4/1/19 12:55 PM, Martin Buchholz wrote:
>>> The spec says that "".hashCode() must be 0.
>>> https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#hashCode()
>>>
>>>
>>>
>>> On Mon, Apr 1, 2019 at 12:51 PM >> > wrote:
>>>
>>>     Wouldn't it be better to write a non-0 value when the computed
>>>     hash code
>>>     is 0, so we don't have to recompute it?  Is there some advantage to
>>>     writing 0 instead of any other value, such as 1?
>>>
>>>     dl
>>>
>>>     On 4/1/19 4:57 AM, Claes Redestad wrote:
>>>     > Hi,
>>>     >
>>>     > when a String has a calculated hash code value of 0, we
>>>     recalculate and
>>>     > store a 0 to the String.hash field every time (except for the empty
>>>     > String, which is special cased). To make String objects more
>>>     amenable to
>>>     > storage in shared read-only memory, e.g., CDS archives, we
>>>     should avoid
>>>     > this redundant store.
>>>     >
>>>     > Bug: https://bugs.openjdk.java.net/browse/JDK-8221723
>>>     > Webrev: http://cr.openjdk.java.net/~redestad/8221723/
>>>     >
>>>     > Testing: tier1-3, no regression on existing and new StringHashCode
>>>     > micros
>>>     >
>>>     > /Claes
>>>     >
>>>

Re: RFR: 8221723: Avoid storing zero to String.hash

[John Rose]

On Apr 1, 2019, at 12:50 PM, dean.l...@oracle.com wrote:
> 
> Wouldn't it be better to write a non-0 value when the computed hash code is 
> 0, so we don't have to recompute it?  Is there some advantage to writing 0 
> instead of any other value, such as 1?

Zero is the easiest sentinel value because it's the default
value for int.

*Any* 32-bit sentinel you choose will be the legitimate hashCode
of *some* string.  That is to say, the range of String.hashCode
is all 2^32 points of the int domain.  That's why we need an extra
bit somewhere if we want to distinguish a sentinel (like zero) from
an identical legitimate hash code that has been cached.

Luckily, there is plenty of "slack" in the memory layout of String.
We can find a bit, if we want to go there.

— John

Re: RFR: 8221723: Avoid storing zero to String.hash

[Peter Levart]

On 4/1/19 10:44 PM, Claes Redestad wrote:

We actually looked at this idea earlier today, and squeezing a "not-
computed" value into String might be "free" since there seems to be a
padding gap on all VM varieties (at least according to JOL estimates[1])

That'd be a larger endeavor, though, since there are places in VM that
calculates and injects the String.hash value.


In addition, this might not be easy from another point of view. You all 
know that there's no synchronization performed when caching the hash 
value. This works, because 32 bits are always read or written 
atomically. If there was another byte to be read or written together 
with 32 bit value, it would be tricky to get the ordering right and 
still keep the performance.


Regards, Peter



/Claes

[1] https://openjdk.java.net/projects/code-tools/jol/

On 2019-04-01 22:02, dean.l...@oracle.com wrote:
OK, I guess there's no ideal solution. Adding a separate 
"not-computed" boolean adds space, and using a different sentinel 
value for "not-computed" would probably be slower on CPUs that have a 
fast compare-and-branch-against-zero instruction.


dl

On 4/1/19 12:55 PM, Martin Buchholz wrote:

The spec says that "".hashCode() must be 0.
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#hashCode() 




On Mon, Apr 1, 2019 at 12:51 PM > wrote:


    Wouldn't it be better to write a non-0 value when the computed
    hash code
    is 0, so we don't have to recompute it?  Is there some advantage to
    writing 0 instead of any other value, such as 1?

    dl

    On 4/1/19 4:57 AM, Claes Redestad wrote:
    > Hi,
    >
    > when a String has a calculated hash code value of 0, we
    recalculate and
    > store a 0 to the String.hash field every time (except for the 
empty

    > String, which is special cased). To make String objects more
    amenable to
    > storage in shared read-only memory, e.g., CDS archives, we
    should avoid
    > this redundant store.
    >
    > Bug: https://bugs.openjdk.java.net/browse/JDK-8221723
    > Webrev: http://cr.openjdk.java.net/~redestad/8221723/
    >
    > Testing: tier1-3, no regression on existing and new 
StringHashCode

    > micros
    >
    > /Claes
    >

Re: RFR: 8216539: tools/jar/modularJar/Basic.java timed out

[Lance Andersen]

 A follow-up on this.

I ran this test 300+ times without failure on the internal Mach 5 machines 
including the one that it failed on (which was only 4 times since January).  
This one system would run the test in approximately 6-7 minutes on average 
where as the the other window systems were running running around  1 - 1.5 
minutes on average.

After a side-bar conversation with Mandy/Alan it was  suggested to  try to  use 
the ToolProvider API vs ProcessBuilder where applicable for invoking the  javac 
and jar commands.

With the change, the system that was running 6-7 minutes was down to 28 seconds 
on average on the slow machine.


The webrev can be found at: 
http://cr.openjdk.java.net/~lancea/8216539/webrev.01/index.html



> On Mar 27, 2019, at 8:23 PM, Mandy Chung  wrote:
> 
> 
> 
> On 3/27/19 4:56 PM, Lance Andersen wrote:
>> Hi Mandy,
>> 
>> 
>>> On Mar 27, 2019, at 7:23 PM, Mandy Chung >> > wrote:
>>> 
>>> Hi Lance,
>>> 
>>> Do you understand what takes so long for this test to run?
>> 
>> Well it is executing a lot of jar commands.  I did not see anything that 
>> sticks out in the failed logs that point to anything obvious.
> 
> One thing you could do is to instrument the test to gather the timing 
> statistics.
> 
>>> Is it running fastdebug and -Xcomp or other flag?
>> 
>> It is just a standard windows run.
> 
> This is even strange if it's running normal product build.
> 
> Mandy
> 
>>> 
>>> Mandy
>>> 
>>> On 3/27/19 1:55 PM, Lance Andersen wrote:
 Hi all ,
 
 Please review this fix for 
 https://bugs.openjdk.java.net/browse/JDK-8216539 
  which increases the 
 timeout value for this test which fails once in a blue moon on windows.
 
 
 ———
 $ hg diff
 diff -r dc66ada06693 test/jdk/tools/jar/modularJar/Basic.java
 --- a/test/jdk/tools/jar/modularJar/Basic.java Tue Mar 26 15:36:19 
 2019 -0700
 +++ b/test/jdk/tools/jar/modularJar/Basic.java Wed Mar 27 16:50:53 
 2019 -0400
 @@ -54,7 +54,7 @@
   *jdk.test.lib.util.FileUtils
   *jdk.test.lib.JDKToolFinder
   * @compile Basic.java
 - * @run testng Basic
 + * @run testng/timeout=300 Basic
   * @summary Tests for plain Modular jars & Multi-Release Modular jars
   */
  
 
 ——
 
 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  
  
 
 
 
 
 
   
 
   
  
  
 
   
 Lance Andersen| 
 Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering 
 1 Network Drive 
 Burlington, MA 01803
 lance.ander...@oracle.com  
  
 
 
 
>>> 
>> 
>>  
>> 
>>   
>> 
>>  Lance Andersen| 
>> Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering 
>> 1 Network Drive 
>> Burlington, MA 01803
>> lance.ander...@oracle.com 
>> 
>> 
>> 
> 

 
  

 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: 8216539: tools/jar/modularJar/Basic.java timed out

[Mandy Chung]

On 4/1/19 5:22 PM, Lance Andersen wrote:

 A follow-up on this.

I ran this test 300+ times without failure on the internal Mach 5 
machines including the one that it failed on (which was only 4 times 
since January).  This one system would run the test in approximately 
6-7 minutes on average where as the the other window systems were 
running running around  1 - 1.5 minutes on average.


After a side-bar conversation with Mandy/Alan it was  suggested to 
 try to  use the ToolProvider API vs ProcessBuilder where applicable 
for invoking the  javac and jar commands.


With the change, the system that was running 6-7 minutes was down to 
28 seconds on average on the slow machine.




This is very good improvement.


The webrev can be found at: 
http://cr.openjdk.java.net/~lancea/8216539/webrev.01/index.html




Looks okay to me.

Mandy

Re: RFR: 8216539: tools/jar/modularJar/Basic.java timed out

[Brian Burkhalter]

> On Apr 1, 2019, at 6:03 PM, Mandy Chung  wrote:
> 
> On 4/1/19 5:22 PM, Lance Andersen wrote:
>>  A follow-up on this.
>> 
>> I ran this test 300+ times without failure on the internal Mach 5 machines 
>> including the one that it failed on (which was only 4 times since January).  
>> This one system would run the test in approximately 6-7 minutes on average 
>> where as the the other window systems were running running around  1 - 1.5 
>> minutes on average.
>> 
>> After a side-bar conversation with Mandy/Alan it was  suggested to  try to  
>> use the ToolProvider API vs ProcessBuilder where applicable for invoking the 
>>  javac and jar commands.
>> 
>> With the change, the system that was running 6-7 minutes was down to 28 
>> seconds on average on the slow machine.
>> 
> 
> This is very good improvement.

+1

>> 
>> The webrev can be found at: 
>> http://cr.openjdk.java.net/~lancea/8216539/webrev.01/index.html 
>> 
>> 
> 
> Looks okay to me.

+1

Brian

[JDK 13] RFR 8178335: fake pass: jdk/internal/ref/Cleaner/ExitOnThrow.java

[Amy Lu]

Please review the patch to fix a fake passed test:

jdk/internal/ref/Cleaner/ExitOnThrow.java

It expects the target test program exists with '1', and throws an 
exception. Due to the target test program be wrongly forked (missed 
--add-exports ), it exists with '1' but with unexpected 
java.lang.IllegalAccessError.


bug: https://bugs.openjdk.java.net/browse/JDK-8178335
webrev: http://cr.openjdk.java.net/~amlu/8178335/webrev.00

Thanks,
Amy