Re: RFR(s): 8023541 Race condition in rmid initialization

[Daniel Fuchs]

Hi Stuart,

I wonder whether you should replace the assert in the
constructor by an explicit null check:

-  assert systemStub != null
+ if (systemStub == null) throw new NullPointerException();

The reason I see is that before your change, an object constructed
with a null systemStub would have sooner or later failed in NPE.
Now with your change, an object constructed with a null system
stub will block - waiting forever for system stub to become not
null.

The question of course is whether throwing NPE in  the constructor
would cause any compatibility issues. Passing the JCK might help
to figure it out.

Best regards,

-- daniel

On 1/30/14 3:57 AM, Stuart Marks wrote:
Hi all, wow, lots of comments on this. Let me see if I can tackle them 
in one message.


Quick aside before I get to the issues: my priorities for this code 
are correctness and maintainability, possibly at the expense of 
performance. In other words I'm willing to make the code more complex 
so that it's correct, but I'm less willing to make it more complex in 
order to make it go faster.


(Tristan, David) Making 'initialized' be volatile. As things stand, as 
David has pointed out (thanks!) it's not necessary for it to be 
volatile. There are other circumstances (see below) where it would be 
necessary to make it volatile, though.


(Alan, Paul) We could convert to double-checked locking and avoid a 
synchronization in the common case, paying only a volatile read. 
Something like,


volatile boolean initialized = false;
...
private void awaitInitialized() {
if (!initialized) {
synchronized (this) {
try {
while (!initialized) {
wait();
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
}
}
}
   }

I *think* that's right. (Is it?) I don't know whether this performs 
any better, or if it does, whether it's worth the additional 
complexity. I had to squint at this for a while to convince myself 
it's correct.


I am fairly sure this is not a performance-critical area of code. 
(Famous last words, I know.) The other threads that can be active here 
are handling remote calls, so they're already doing network I/O, 
unmarshalling, and dispatching to the RMI thread pool. Compared to 
this, the incremental cost of a synchronization block seems 
inconsequential. I don't have much intuition about how much we'd save 
by substituting a volatile read for a full synchronization in the 
common case, but given that this is within the context of a fairly 
expensive operation, it doesn't seem like it's worth it to me.


(Alan, Paul) Calling awaitInitialized isn't strictly necessary 
anywhere except for the equals(NAME) case of lookup(). Yes, that's 
right. I added it everywhere because of a possibly misguided sense of 
completeness and consistency. One could essentially redefine 
awaitInitialized() to protect just the systemStub field, not the 
"entire" object, whose only state is that field anyway. Also, see 
below regarding renaming this method.


(Alan) Use systemStub == null as the condition instead of 
!initialized. I considered at first this but it got confusing really 
fast. Take a look:


private final ActivationSystem systemStub;

SystemRegistryImpl(..., systemStub) {
...
this.systemStub = systemStub;
notifyAll();
...
}

private synchronized void awaitInitialized() {
...
while (systemStub == null) {
wait();
}
...
}

We rely on systemStub to be initialized at object creation time 
(before construction!) to its default value of null. I think this is 
right. The constructor then initializes the blank final to non-null 
and notifies.


Then, awaitInitialized seems straightforward until you see that the 
condition is waiting for the value of a final variable to change! JLS 
sec 17.5 [1] allows all sorts of optimizations for final fields, but 
they all are qualified with what is essentially a safe publication 
requirement on the reference:


An object is considered to be completely initialized when its 
constructor
finishes. A thread that can only see a reference to an object 
after that
object has been completely initialized is guaranteed to see the 
correctly

initialized values for that object's final fields.

[1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5

Unfortunately this code *unsafely* publishes a reference to 'this' 
which is the root of this whole problem. Under these circumstances I'd 
prefer to be really conservative about the code here. I can't quite 
convince myself that a condition loop waiting for a final field to 
change value is safe. That's why I added a separate field.


I can see removing the boolean and using systemStub == null as the 
condition making things simpler, since there are fewer variables to 
reason a

Re: RFR(s): 8023541 Race condition in rmid initialization

[David Holmes]

On 30/01/2014 5:34 PM, Tristan Yan wrote:

Hi Stuart
I didn’t make my comment clear. You set interrupted as true when thread
gets interrupted. Here it's still going to wait until systemStub is not
null. Why do you still need interrupt current thread in this case.


Because the golden rule of interrupt handling is you either throw 
InterruptedException or you re-assert the interrupted state. This allows 
code further up the stack to respond to interrupts.


David


Thank you
Tristan

On Jan 30, 2014, at 11:24 AM, David Holmes mailto:david.hol...@oracle.com>> wrote:


http://cr.openjdk.java.net/~smarks/reviews/8023541/webrev.1/

Re: RFR(s): 8023541 Race condition in rmid initialization

[Paul Sandoz]

On Jan 30, 2014, at 3:57 AM, Stuart Marks  wrote:
> 
> Then, awaitInitialized seems straightforward until you see that the condition 
> is waiting for the value of a final variable to change! JLS sec 17.5 [1] 
> allows all sorts of optimizations for final fields, but they all are 
> qualified with what is essentially a safe publication requirement on the 
> reference:
> 
>An object is considered to be completely initialized when its constructor
>finishes. A thread that can only see a reference to an object after that
>object has been completely initialized is guaranteed to see the correctly
>initialized values for that object's final fields.
> 
> [1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5
> 
> Unfortunately this code *unsafely* publishes a reference to 'this' which is 
> the root of this whole problem. Under these circumstances I'd prefer to be 
> really conservative about the code here. I can't quite convince myself that a 
> condition loop waiting for a final field to change value is safe. That's why 
> I added a separate field.
> 

I think you have done the right thing in the latest webrev, even though i 
suspect the runtime does not fully optimize final fields as constants (since it 
is still possible to update final fields, e.g. see System.out).

It should not be this hard to reason about this stuff, right?

Hopefully updates to the JMM will make this easier to grok, even though this is 
a naughty case.

Paul.

hg: jdk8/tl/nashorn: 8032944: Improve reflection in Nashorn

[sundararajan . athijegannathan]

Changeset: a43c125b03dc
Author:sundar
Date:  2014-01-30 18:34 +0530
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/a43c125b03dc

8032944: Improve reflection in Nashorn
Reviewed-by: jlaskey, attila, ahgross

! src/jdk/nashorn/internal/objects/NativeObject.java
+ test/script/sandbox/classbind.js

hg: jdk8/tl/nashorn: 8032954: Nashorn: extend Java.extend

[sundararajan . athijegannathan]

Changeset: eca774d33fa4
Author:sundar
Date:  2014-01-30 19:04 +0530
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/eca774d33fa4

8032954: Nashorn: extend Java.extend
Reviewed-by: attila, jlaskey, ahgross

! src/jdk/nashorn/internal/runtime/linker/JavaAdapterFactory.java
! test/script/sandbox/classbind.js
! test/script/sandbox/classloader.js
! test/script/sandbox/classloader.js.EXPECTED

Re: [8011944] Sort fails with ArrayIndexOutOfBoundsException

[Alan Bateman]

On 30/01/2014 02:34, Miroslaw Niemiec wrote:
The copyright header added to 
test/java/util/Arrays/TimSortStackSize.java:

http://cr.openjdk.java.net/~miroslawzn/8011944/webrev.02/

- Miroslaw

Thanks, just a few minor nits to look at before you push this:

- The import at L30 is mis-aligned.
- The creation of the Compatator at L49 looks like a new statement, not 
immediately obvious that it's a parameter to sort (so best to reformat it).

- The copyright date says 2005 so I assume it was copied from another file.

-Alan

Re: RFR(s): 8023541 Race condition in rmid initialization

[Alan Bateman]

On 30/01/2014 09:09, Daniel Fuchs wrote:

Hi Stuart,

I wonder whether you should replace the assert in the
constructor by an explicit null check:

-  assert systemStub != null
+ if (systemStub == null) throw new NullPointerException();

The reason I see is that before your change, an object constructed
with a null systemStub would have sooner or later failed in NPE.
Now with your change, an object constructed with a null system
stub will block - waiting forever for system stub to become not
null.
I suspect the system stub can never be null but I agree it should be 
checked before creating the RegistryImpl. One way to do that is by going 
through another constructor, something like this:


private SystemRegistryImpl(int port,
   RMIClientSocketFactory csf,
   RMIServerSocketFactory ssf,
   ActivationSystem systemStub,
   Void unused) throws RemoteException {
super(port, csf, ssf);
synchronized (this) {
this.systemStub = systemStub;
notifyAll();
}
}

SystemRegistryImpl(int port,
   RMIClientSocketFactory csf,
   RMIServerSocketFactory ssf,
   ActivationSystem systemStub) throws 
RemoteException {

this(port, csf, ssf, Objects.requireNonNull(systemStub), null);
}

but maybe that it too subtle.

-Alan

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Alan Bateman]

On 29/01/2014 19:10, Mandy Chung wrote:


On 1/29/2014 5:09 AM, Peter Levart wrote:


Since I don't know what should be the correct behaviour of javac, I 
can leave the Reference.java changes as proposed since it compiles in 
both cases. Or should I revert the change to declaration of local 
variable 'q' ? 


I slightly prefer to revert the change to ReferenceQueueObject> for now as there is no supertype for Object and this looks a 
little odd.  We can clean this up as a separate fix after we get 
clarification from compiler-dev.
I see Peter has posted a question to compiler-dev on this and it can 
always be re-visited once it clear why it compiles when both Reference 
and ReferenceQueue are in the same compilation unit.


-Alan

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Peter Levart]

On 01/30/2014 03:46 PM, Alan Bateman wrote:

On 29/01/2014 19:10, Mandy Chung wrote:


On 1/29/2014 5:09 AM, Peter Levart wrote:


Since I don't know what should be the correct behaviour of javac, I 
can leave the Reference.java changes as proposed since it compiles 
in both cases. Or should I revert the change to declaration of local 
variable 'q' ? 


I slightly prefer to revert the change to ReferenceQueueObject> for now as there is no supertype for Object and this looks a 
little odd.  We can clean this up as a separate fix after we get 
clarification from compiler-dev.
I see Peter has posted a question to compiler-dev on this and it can 
always be re-visited once it clear why it compiles when both Reference 
and ReferenceQueue are in the same compilation unit.


-Alan


I Just commited the version with no change to ReferenceQueue 
line to jdk9/dev. If there is a bug in javac and the code would not 
compile as is, the change to this line should be committed as part of 
javac fix, right?


Regards, Peter

hg: jdk8/tl/nashorn: 8032949: Nashorn linkages awry

[sundararajan . athijegannathan]

Changeset: c59fb10cb0b5
Author:sundar
Date:  2014-01-30 19:45 +0530
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/c59fb10cb0b5

8032949: Nashorn linkages awry
Reviewed-by: jlaskey, attila, ahgross

! src/jdk/nashorn/internal/objects/NativeObject.java
! src/jdk/nashorn/internal/runtime/linker/Bootstrap.java
! src/jdk/nashorn/internal/runtime/linker/JavaAdapterFactory.java
! src/jdk/nashorn/internal/runtime/linker/NashornStaticClassLinker.java
! src/jdk/nashorn/internal/runtime/linker/ReflectionCheckLinker.java
! test/src/jdk/nashorn/api/scripting/ScriptEngineSecurityTest.java
! test/src/jdk/nashorn/api/scripting/ScriptEngineTest.java

Re: [8011944] Sort fails with ArrayIndexOutOfBoundsException

[Miroslaw Niemiec]

On 1/30/2014 5:46 AM, Alan Bateman wrote:

On 30/01/2014 02:34, Miroslaw Niemiec wrote:
The copyright header added to 
test/java/util/Arrays/TimSortStackSize.java:

http://cr.openjdk.java.net/~miroslawzn/8011944/webrev.02/

- Miroslaw

Thanks, just a few minor nits to look at before you push this:

- The import at L30 is mis-aligned.
- The creation of the Compatator at L49 looks like a new statement, 
not immediately obvious that it's a parameter to sort (so best to 
reformat it).
- The copyright date says 2005 so I assume it was copied from another 
file.


-Alan

Hi Alan,

I have corrected the test file according to your suggestions
The new webrev:
http://cr.openjdk.java.net/~miroslawzn/8011944/webrev.03/

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Alan Bateman]

On 30/01/2014 14:51, Peter Levart wrote:


I Just commited the version with no change to ReferenceQueue 
line to jdk9/dev. If there is a bug in javac and the code would not 
compile as is, the change to this line should be committed as part of 
javac fix, right?


It's good to get this change in. If javac were to be changed to reject 
this code then it need to be changed at the same time (but I guess we 
wait to see if this is case as it's just not obvious yet).


-Alan

Re: [8011944] Sort fails with ArrayIndexOutOfBoundsException

[Alan Bateman]

On 30/01/2014 15:36, Miroslaw Niemiec wrote:

Hi Alan,

I have corrected the test file according to your suggestions
The new webrev:
http://cr.openjdk.java.net/~miroslawzn/8011944/webrev.03/


Looks good, thanks for this.

This backport reminds me that maybe it is time to consider removing the 
merge sort code (and the system property to select it) in 9.


-Alan.

Re: RFR: 8030822: (tz) Support tzdata2013i

[Michael Fang]

Looks good to me too.

thanks,

-michael

On 14年01月29日 10:14 下午, Masayoshi Okutsu wrote:

Looks good.

Masayoshi

On 1/30/2014 5:31 AM, Aleksej Efimov wrote:

Masayoshi, Sean,
Thank you for the review and your comments.

I have prepared a second version of the fix [1] without the 
.properties files. These files are used in 
test/sun/util/resources/TimeZone/TimeZoneNames/TimeZoneNamesTest.java 
test and because of that this test was removed also.


Thank you,
Aleksej

[1] http://cr.openjdk.java.net/~aefimov/8030822/9/webrev.01/ 




On 01/23/2014 08:43 AM, Masayoshi Okutsu wrote:

Hi Aleksej,

I think this one is the first tzdata change for JDK9. So I'd suggest 
that all the TimeZoneNames*.properties files, which were temporarily 
created for the translation work, be removed.


Thanks,
Masayoshi

On 1/21/2014 10:17 PM, Aleksej Efimov wrote:

Hi,
Can I have a review for 2013i timezone data integration [1] to JDK9.
There is one change in tzdb that affects naming for "Asia/Amman" 
timezone:
"The Jordan switches back to standard time at 00:00 on December 20, 
2013."
All changes in TimeZoneNames*.java and TimeZoneNames*.properties 
files are related to this one tzdb change.


The list of executed regression test sets: test/sun/util/calendar 
test/java/util/Calendar test/sun/util/resources/TimeZone 
test/sun/util/calendar test/java/util/TimeZone test/java/time 
test/java/util/Formatter test/closed/java/util/Calendar 
test/closed/java/util/TimeZone

All tests gave PASS result.

Webrev location: 
http://cr.openjdk.java.net/~aefimov/8030822/9/webrev.00/


Thank you,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8030822

Re: StringJoiner: detect or fix delimiter collision?

[Philip Hodges]

I did not predict that it would be a "sprintf". It's not going to be 
consistently misused anything like so frequently.
I compared it to the unescaped XML generation antipattern.

I have not seen any technical justifications whatsoever so far, just 
inexplicable enthusiasm.

It is like giving young drivers a faster car with no seat belts.

The trouble is, unlike in Google guava, this is the JDK, you can't just drop 
flawed beta interfaces two versions later. All you can do is add them to static 
review tool blacklists and deprecate them to cause a warning at compile time, 
and hope that people will give them a wide berth. Even if you hide them in an 
unoffical sun.com package. By the way, I'm looking forward to the proprietary 
Base64 being changed to simply call the new official one. You can't even modify 
an equals method in an undocumented reflection implementation in 1.7.0.51 
without breaking production applications. Wow.

I am raising doubt and you are ignoring it.
Don't you have the guts to say whoa, stop the bandwagon, there could just be a 
real problem with this, it really will make it easier to create bugs without 
any warning from the compiler and without making anyone's code better *in any 
way at all*?

I am picking on String.join precisely because I have seen way too many 
delimiter collision bugs, not just in Java but in several other languages, and 
because this interface perpetuates a real world problem by preventing future 
interface changes to detect them.

[I am not always a doomsayer. For example, I am not picking on JSR310 because 
Date and Calendar and SimpleDateFormat are well known disaster areas and the 
people working on their replacement have a deep understanding of the issues, 
have really taken their time, and nothing whatsoever that I have read about it 
jumped out at me and said this is *wrong*. It might not even be completely 
perfect. But I am confident it will be so much better than what we have now, 
and it's a shame that I won't have time to migrate existing apps to it.]

The counterproposal, no, proposal refinement, wafting around inside my head, is 
to somehow compel programmers to make just one more method call before that 
final toString(). It will be difficult to find good names, especially ones that 
will be understood by programmers for whom English is not a first language. 
Something like:

String.join(delimiter, joinables).assertNoUnescapedDelimiters().toString();
String.join(delimiter, joinables).neverMindDelimiterCollisions().toString();
String.join(delimiter, joinables).promiseNoUnescapedDelimiters().toString();
String.join(delimiter, joinables).escapeDelimiters(escapeChar).toString();
String.join(delimiter, joinables).quoteElements(quoteChar).toString();


The vital thing is that String.join has to return an unjoinable, that needs an 
adapter method to make it safely joinable. If you get that right, then we can 
forgive this first shot being a little slow, and enjoy the triumph of 
CharSequence over immutability.

Yes, ultimately the goal should be to add full support for at least the most 
popular csv generation recipes.

I'm really sorry I couldn't carry on arguing the case before August. As a 
minority, I only have one person's quota of energy. I will try to get some more 
people to look at it.


On 27 Jan 2014, at 18:44, Mike Duigou  wrote:

> 
> On Jan 26 2014, at 17:12 , Philip Hodges  wrote:
> 
>> Please please please drop StringJoiner from Java 1.8 before it is too late.
> 
> It is well past "too late". The API has been frozen since August for all but 
> the most exceptional cases.
> 
 At first I thought they were cool. Then I tried to use them in anger.
 And was forced to roll my own.
>>> 
>>> I would encourage you to share your implementation or the javadocs as grist 
>>> for discussion.  An actual alternative is the *only* thing that is likely 
>>> to be persuasive.
> 
> You seem to be very much in the minority on this issue by the silence from 
> other responders on this list and elsewhere. The concerns you've highlighted 
> with regard to delimiter management, while certainly valid, have not been of 
> sufficient concern to suggest a change in the proposal. The prediction that 
> using StringJoiner will become Java's sprintf seems unlikely to be be true. 
> The reception we've seen thus far for StringJoiner has been otherwise 
> exclusively enthusiastic and positive. 
> 
> Alternatives, refinements and counterproposals are always welcome in the 
> discussion. Doomsaying unfortunately adds little.
> 
> Mike

Re: StringJoiner: detect or fix delimiter collision?

[Philip Hodges]

Please please please drop StringJoiner from Java 1.8 before it is too late.

It is not needed. It does not add value. It is an embarrassment.
We did without it for years. It is not long long overdue. We do not need it now.
It does not need to be in the very first Java 1.8 release.
Try leaving it out, and see if people even complain.
Most people won't even notice it, let alone be ready to misuse it straight away.

String.join is slower than Arrays.toString() or Collections.toString(). Test 
failed.
String.join(delimiter, iterable) needs extra code to call toString() if 
elements are not already CharSequence. Convenience? Test failed.
StringJoiner does not force or even remind the developer to consider delimiter 
collisions. Test failed.

It is a perfect complement for split: the hard to use interface that can't 
split the joined input reliably because it might have too many delimiters.

It will be the newest star on java antipattern web sites. A good place would be 
just after the item about not "Assembling XML with String operations" because 
the text parts might contain special characters.

Even if it ever did appear that StringJoiner and String.join did what we wanted,
I would still much rather roll my own than use the new StringJoiner in the 
library.
Then I could set the initial capacity of the StringBuilder, skip the defensive 
copies and null checks, and above all, introduce some extra mandatory method 
call to force the developer to tell the compiler what is being done about 
delimiter collisions, even if it is to add an assert that there are no 
delimiters in the added CharSequences, or to waive that check because numbers 
never never contain comma delimiters (hold on ... better check the locale to 
see if comma is used as a decimal point or grouping character after all).

I came across this non-justification [with my additions] on a blog:

"StringJoiner and String.join(...) are long, long overdue. They are so long 
overdue that the vast majority of Java developers likely have already written 
or have found [or have had to mend] utilities for joining strings [to make an 
unsplittable delimiter collision], but it is nice [or at least well-meant] for 
the JDK to finally provide this itself. Everyone has encountered situations 
where joining strings is required [and they forgot all about escaping, or 
should have used a PreparedStatement instead], and it is a Good Thing™ that we 
can now express that [bug opportunity] through a standard API that every [no, 
just some] Java developer (eventually) will know [of, but not actually use, and 
even if they do they will need an IDE to tell them that the prefix comes 
*after* the delimiter]."



> To quote Joshua Bloch on good API design: 'When in doubt leave it out' (or 
> 'You can always add, but you can never remove')



> 
> 
> On 31 Aug 2013, at 23:13, Mike Duigou  wrote:
> 
> 
> On Aug 30 2013, at 23:40 , Philip Hodges wrote:
>> ...
>> 
>> Why is there such a bandwagon rolling for this "convenience" feature?
> 
> Perhaps others just don't agree with you. The choice of functionality offered 
> in the JDK 8 StringJoiner was not arbitrary or haphazardly chosen. If it 
> doesn't meet your particular needs, I am sorry to hear that. Our belief as 
> the proposers is that it offers a reasonable mix of functionality and 
> simplicity to cover useful number of requirements. Our intent was certainly 
> not to create the kitchen magician [1] of string joiners that attempted to 
> incorporate every conceivable option. In particular your concerns about 
> escaping are out of scope for the joiner in part because
> 
> Collection strings;
> 
> String concat = 
> strings.stream().map(MyUtils::escape).collect(Collections.joining(","));
> 
> seems like an adequate and flexible solution to most people.
> 
>> We already have joiners in apache commons and guava.
> 
> This is the reason that StringJoiner was considered for JDK--that many others 
> were "rolling their own".
> 
>> At first I thought they were cool. Then I tried to use them in anger.
>> And was forced to roll my own.
> 
> I would encourage you to share your implementation or the javadocs as grist 
> for discussion.  An actual alternative is the *only* thing that is likely to 
> be persuasive.
> 
>> That can't be right.
> 
> Sometimes it is. The JDK doesn't make any attempt to satisfy everyone (or 
> anyone). We all end up writing lots of non-business logic utility functions 
> to bridge gaps in what JDK offers. This is normal. And, if it turns out to be 
> something lots of people need then it's entirely possible that it could be 
> added to the JDK.
> 
> Mike
> 
> [1] https://www.youtube.com/watch?v=cGVG9xLHb84
> 
>> 
>> A more elaborate offically blessed feature that
>> only does half the job is worse than useless.
>> Without the extra complex ability to detect or avoid collisions
>> it is neither "nice", nor a "Good Thing".
>> 
>> Phil
>> 
>> http://www.techempower.com/blog/2013/03/26/everything-about-java-8/

Re: Which optimizations does Hotspot apply?

[Andrej Golovnin]

Hi Rémi,

> With latest jdk8, it's not true anymore.
> (and most of the time the iterator object is not created anymore at least 
> with jdk7+).

Could you please explain it a little bit more? When is that optimization 
applied,
e.g. what conditions are required for this optimization, since which version
of JDK/Hotspot it is supported, where it is implemented in JDK?

When I take look at a product I'm working on, I see a lot instances of 
ArrayList$Itr objects,
which are created by for-each loops (we use JDK 7u51).

Thanks in advance!

Best regards,
Andrej Golovnin

[8011944] Sort fails with ArrayIndexOutOfBoundsException

[Miroslaw Niemiec]

Hello!

This is a simple backport from 8 to 7.
Looking for a review of this even though it only requires a testcase 
change due to the use of lambda expressions.
Since this is the first of these I've encountered I thought I better 
play it safe, but generally speaking, are we ok
to skip the review process for backports like this? (minor lambda 
related testcase changes - if the lambda's are in the actual fix it 
probably makes sense to re-review).


The fix:
src/share/classes/java/util/ComparableTimSort.java
src/share/classes/java/util/TimSort.java

applied with no modification to 7

The only simple change was to replace a lambda expression in test case
test/java/util/Arrays/TimSortStackSize.java:


Arrays.sort(genData(), Integer::compare);

got replaced with

Arrays.sort(genData(),
new java.util.Comparator() {
   public int compare(Integer a1, Integer a2){
 return Integer.compare(a1.intValue(), a2.intValue());
   }
});


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

Original fix for 8:
http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/07585a2483fa

Fix for 7:
http://cr.openjdk.java.net/~miroslawzn/801144/webrev.01/


Thank you
Miroslaw




 

Re: JEP 187: Serialization 2.0 & Serialization-aware constructors

[Robert Stupp]

Am 22.01.2014 um 17:33 schrieb "David M. Lloyd" :

> The concept of explicit deserialization constructors is interesting and is 
> something I've explored a little bit in the context of JBoss Marshalling.
...
> The idea with a serialization-aware constructor is that each serializable 
> class constructor can read the stream information itself and initialize 
> fields the "normal" way, with "normal" validation.

I agree with David.

One thing to think about might be to introduce a special kind of constructor by 
extending the JLS - to introduce a special "deserialization constructor" that 
behaves a bit different than "normal" constructors.
Deserialization-constructors are called "top-down" (java.lang.Object first) - 
the deserialization code is responsible to call the 
deserialization-constructors for each individual class.
The deserialization-constructor receives information only for deserialized 
fields that the class "owns" (not for fields of any superclass or subclass).
But: deserialization-constructors should not be callable from "normal" code.

Maybe there could be multiple "deserialization constructors" - one per 
serialVersionUID. This should completely eliminate "spaghetti code" in 
"readObject()" and the need for "readObject()" at all.

Another idea is to introduce a kind of "protocol" how to deal with incompatible 
class changes or serial-version-uid mismatches. This gets important when 
existing applications grow: how shall a new field be initialized, when it's not 
present in input stream? Maybe this could be handled by the "deserialization 
constructor".
Regarding the situation when an older implementation reads data from a newer 
implementation (with more fields or changed fields) ... I think that special 
situation cannot be handled. But it might be possible to serialize objects 
explicitly for older versions - for example if the implementation knows that it 
creating an RPC response for an older client. writeObject() could be replaced 
with "serialization methods" - multiple "serialization methods" could be used 
to address different versions.

A new serialization framework should also add support for classes with "factory 
methods" like those with static "valueOf(...)" methods - maybe in combination 
to replace readResolve/writeReplace.
Serialization should also respect immutable objects like String, 
Byte/Integer/Long/..., InetAddress, UUID - this reduces the serialized payload 
and amount of (unnecessary) objects during deserialization.

Maybe serialization should move a bit deeper into the JVM itself - to optimize 
the reflection/unsafe stuff.

IMHO the explicit serialFields support should not be supported for 
"serialization 2.0" - maybe I'm wrong, but I do not know any code that uses 
this (except some core JDK classes) - for me it looks like a workaround to deal 
with incompatible class changes.

Transport and serialization might be separated in "serialization 2.0". Current 
OOS/OIS implementations perform a lot of System.arraycopy operations and 
allocate a lot of temporary objects (e.g. during string (de)serialization). It 
should be possible to "feed" deserialization with ByteBuffers - for example by 
using NIO. For example: the current internal "readPrimitives()" method could 
directly operate on a ByteBuffer without any copy operation - maybe with native 
support from the JVM.

Robert

Re: Which optimizations does Hotspot apply?

[David Holmes]

Hotspot questions belong on hotspot lists. cc'ing hotspot-dev and 
(trying to) bcc core-libs-dev.


David

On 23/01/2014 9:37 AM, Remi Forax wrote:

On 01/22/2014 11:37 PM, Robert Stupp wrote:

Is there any documentation available which optimizations Hotspot can
perform and what "collecting a garbage" object costs?
I know that these are two completely different areas ;)

I was inspecting whether the following code
 for (Object o : someArrayList) { ... }
would be faster than
 for (int i=0, l=someArrayList.size(); i

For a long time, using a for with an index (if you are *sure* that it's
an ArrayList) was faster.
With latest jdk8, it's not true anymore.
(and most of the time the iterator object is not created anymore at
least with jdk7+).



For example:
- Does it help Hotspot to declare parameters/variables as "final" or
can Hotspot identify that?


no, it's an urban myth.
You can test it by yourself, if you declare a local variable final or
not the exact same bytecode is produced by javac. The keyword final for
a local variable (or a parameter) is not stored in the bytecode.

BTW, final was introduced in 1.1 mostly to allow to capture the value of
a variable to be used by an anonymous class, Java 8 doesn't require this
kind of variable to be declared final anymore.


- When does Hotspot inline method calls in general and getters/setters
especially?


In general, up to a depth of 10 by default and 1 for a recursive method.
Roughly, a method call is not inlined either if the call is virtual and
can call too many implementations or if the generated assembly code will
be too big.



I think such a piece of documentation (just what Hotspot can do in
which release) would be really helpful when someone tries to optimize
code - want to say: the question is: "Is something worth to spend time
on or is this special situation handled by Hotspot?"


It never worth it.
Choose the right algorithms and shape your data to be easily consumed by
your algorithms is enough.



-Robert


Rémi

Re: RFR(s): 8023541 Race condition in rmid initialization

[Stuart Marks]

On 1/30/14 2:35 AM, David Holmes wrote:

On 30/01/2014 5:34 PM, Tristan Yan wrote:

Hi Stuart
I didn’t make my comment clear. You set interrupted as true when thread
gets interrupted. Here it's still going to wait until systemStub is not
null. Why do you still need interrupt current thread in this case.


Because the golden rule of interrupt handling is you either throw
InterruptedException or you re-assert the interrupted state. This allows code
further up the stack to respond to interrupts.


Right, thanks David.

While it's often acceptable to let InterruptedException propagate, or to catch 
and rethrow it, IE is a checked exception so this isn't always allowed. That's 
the case with this code. The only alternative in this case is to re-assert the 
interrupted state before returning.


There's some discussion of this in Goetz, et. al. "Java Concurrency In 
Practice", section 5.4. There's an example here of catching IE and reasserting 
the thread's interrupted state. Get this book if you don't have it already.


The *worst* thing to do is something like,

try {
... wait or sleep or something that throws IE ...
} catch (InterruptedException ie) {
// ignored
}

I write this all the time in toy programs, but product code and even test code 
should follow the golden rule.


s'marks

Re: RFR(s): 8023541 Race condition in rmid initialization

[Stuart Marks]

On 1/30/14 1:09 AM, Daniel Fuchs wrote:

I wonder whether you should replace the assert in the
constructor by an explicit null check:

-  assert systemStub != null
+ if (systemStub == null) throw new NullPointerException();

The reason I see is that before your change, an object constructed
with a null systemStub would have sooner or later failed in NPE.
Now with your change, an object constructed with a null system
stub will block - waiting forever for system stub to become not
null.

The question of course is whether throwing NPE in  the constructor
would cause any compatibility issues. Passing the JCK might help
to figure it out.


I considered an explicit null test, or something like Objects.requireNonNull, 
but decided against them. The reason is that this is a private nested class, and 
there is only one caller elsewhere in the containing class. We just need to make 
sure this is right, and there's no use checking at runtime.


I was going to add a comment saying "systemStub must be non-null" but then I 
realized Hey! That's what assert statements are for.


Rmid creation always goes through this path, and this is done repeatedly by the 
tests. Tests are run with assertions enabled so I think we're well covered here.


s'marks

hg: jdk8/tl/nashorn: 8032681: Issues with Nashorn

[eric . mccorkle]

Changeset: 11b83c913cca
Author:attila
Date:  2014-01-30 20:14 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/11b83c913cca

8032681: Issues with Nashorn
Reviewed-by: ahgross, jlaskey, sundar

+ src/jdk/internal/dynalink/linker/GuardedTypeConversion.java
! src/jdk/internal/dynalink/linker/GuardingTypeConverterFactory.java
! src/jdk/internal/dynalink/support/LinkerServicesImpl.java
! src/jdk/internal/dynalink/support/TypeConverterFactory.java
! src/jdk/nashorn/api/scripting/NashornScriptEngine.java
! src/jdk/nashorn/internal/objects/NativeJava.java
! src/jdk/nashorn/internal/objects/NativeJavaImporter.java
! src/jdk/nashorn/internal/runtime/Context.java
! src/jdk/nashorn/internal/runtime/NativeJavaPackage.java
! src/jdk/nashorn/internal/runtime/ScriptFunction.java
! src/jdk/nashorn/internal/runtime/linker/AdaptationResult.java
! src/jdk/nashorn/internal/runtime/linker/JSObjectLinker.java
! src/jdk/nashorn/internal/runtime/linker/JavaAdapterBytecodeGenerator.java
! src/jdk/nashorn/internal/runtime/linker/JavaAdapterClassLoader.java
! src/jdk/nashorn/internal/runtime/linker/JavaAdapterFactory.java
! src/jdk/nashorn/internal/runtime/linker/JavaAdapterServices.java
! src/jdk/nashorn/internal/runtime/linker/NashornBottomLinker.java
! src/jdk/nashorn/internal/runtime/linker/NashornLinker.java
! src/jdk/nashorn/internal/runtime/linker/NashornPrimitiveLinker.java
! src/jdk/nashorn/internal/runtime/linker/NashornStaticClassLinker.java
! src/jdk/nashorn/internal/runtime/resources/Messages.properties
! test/script/basic/JDK-8014647.js
! test/script/basic/JDK-8014647.js.EXPECTED
! test/script/basic/javaclassoverrides.js
! test/script/basic/javaclassoverrides.js.EXPECTED
! test/script/sandbox/javaextend.js
! test/script/sandbox/javaextend.js.EXPECTED
! test/src/jdk/nashorn/api/scripting/ScriptEngineSecurityTest.java
+ test/src/jdk/nashorn/test/models/ClassWithFinalFinalizer.java
+ test/src/jdk/nashorn/test/models/ClassWithInheritedFinalFinalizer.java

Re: RFR(s): 8023541 Race condition in rmid initialization

[Stuart Marks]

On 1/30/14 3:13 AM, Paul Sandoz wrote:

On Jan 30, 2014, at 3:57 AM, Stuart Marks  wrote:


Then, awaitInitialized seems straightforward until you see that the
condition is waiting for the value of a final variable to change! JLS sec
17.5 [1] allows all sorts of optimizations for final fields


I think you have done the right thing in the latest webrev, even though i
suspect the runtime does not fully optimize final fields as constants (since
it is still possible to update final fields, e.g. see System.out).


It might not optimize final fields now (I tried running a test with several JVM 
options, but I'm sure there are ones I haven't tried) so things do seem to work. 
However, I couldn't convince myself that a *future* optimization wouldn't cause 
a problem. I envisioned either a protracted email discussion/argument or a hairy 
debugging session. At that point I decided to remove final. :-)


(I'm reminded of the test failures introduced by more-aggressive GC of 
narrowly-scoped local variables.)



It should not be this hard to reason about this stuff, right?

Hopefully updates to the JMM will make this easier to grok, even though this
is a naughty case.


Maybe. I'd guess that the new JMM will stick to covering well-behaved programs 
(e.g. ones that adhere to safe publication). The difficulty with issues like 
this one is that once publication has occurred unsafely, we have to figure out 
how to drag it back into the safe area. There are probably too many ways to 
write unsafe programs for the JMM to cover them in a simple fashion.


s'marks

Re: StringJoiner: detect or fix delimiter collision?

[Henry Jen]

On 01/27/2014 12:31 PM, Philip Hodges wrote:

I did not predict that it would be a "sprintf". It's not going to be 
consistently misused anything like so frequently.
I compared it to the unescaped XML generation antipattern.

I have not seen any technical justifications whatsoever so far, just 
inexplicable enthusiasm.

It is like giving young drivers a faster car with no seat belts.



As Mike had said, delimiter collision is a valid concern, and I think 
your metaphore is quite vivid.


Anyhow, there is need for fast and simply join operation, and this is 
what JDK is intend to provide. Nothing prohibit developer to

- choose a better delimiter for the data
- escape the element before sending to joiner

Since we expect more complicate use case is more likely to be used in 
stream API, such escaping can be easily done by adding a map step.


elements.stream()
.map(escapeFunction)
.collect(joining())

Cheers,
Henry

[8] RFR (S): 8033278: Missed access checks for Lookup.unreflect* after 8032585

[Vladimir Ivanov]

http://cr.openjdk.java.net/~vlivanov/8033278/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8033278

The fix for 8032585 [1] introduced a regression: in some cases access 
check on a reference class is omitted.


The fix is to ensure that access check on a reference class is always 
performed.


Testing: regression test, jdk/test/java/lang/invoke/, 
jdk/test/java/util/stream, vm.defmeth.testlist, vm.mlvm.testlist, 
nashorn (unit tests, octane), groovy


Thanks!

Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~vlivanov/8032585/webrev.00/