Re[2]: Implicit 'this' return for void methods

[Victor Polischuk]

Ulf,

I think that point leading style is something which can be easily mistreat. If 
we complicate the example:

   String mySub = myVeryLongNamedString.substring(.indexOf("C"),.indexOf("Q"));

to something like:

   String mySub = 
myVeryLongNamedString.concat("BLAH").substring(.indexOf("C"),.indexOf("Q"));

it is not 100% clear which object will be applied for ".indexOf" method, either 
(myVeryLongNamedString) or (myVeryLongNamedString + "BLAH"). Also I believe 
that the need of executing the instance methods to get parameter values for its 
another method is an API issue. Helpers like StringUtils from commons-lang 
resolve this particular case and many similar. Moreover, the helper correctly 
takes care of the situation when Q meets before C which means you would not use 
the feature like in above (even if it is implemented).

However, I totally agree that the "chaining" feature should be done as a 
compiler time AST modification to allow using legacy APIs without recompilation 
and keep byte-code, existing APIs and various conventions untouched.

---
Regards,
Victor Polischuk


 --- Original message ---
 From: "Ulf Zibis" 
 Date: 28 March 2014, 00:30:45
  


> 
> Am 27.03.2014 23:05, schrieb Eirik Lygre:
> > With this suggested change, the only behavior that will change is that some 
> > code which used to not 
> > compile will start compiling, with a reasonable result. No code that used 
> > to compile will change.
> 
> Yes, this is one of the great advantages of this "simple" _language_ change 
> proposal !
> 
> -Ulf
> 
> 

Re: Implicit 'this' return for void methods

[Ulf Zibis]

Am 27.03.2014 23:05, schrieb Eirik Lygre:
With this suggested change, the only behavior that will change is that some code which used to not 
compile will start compiling, with a reasonable result. No code that used to compile will change.


Yes, this is one of the great advantages of this "simple" _language_ change 
proposal !

-Ulf

Re: Implicit 'this' return for void methods

[Eirik Lygre]

On Thu, Mar 27, 2014 at 10:02 PM, Jochen Theodorou wrote:

> Am 27.03.2014 21:52, schrieb Eirik Lygre:
> [...]
>
>  The JavaBean specification, with it's "void setSomething()" functions
>> are fundamental to so many things Java that they will never go away
>> (good thing, too!).The suggested language change builds on top of that,
>> is beneficial to a large body of existing code and does not invalidate
>> any existing practices.
>>
>> Hmm... isn't it a problem if I change "void setSomething()" to "MyClass
> setSomething()". In example, is that still a valid setter in terms of the
> JavaBean Spec? I might be wrong, but afaik there are bean based tools out
> there, not recognizing setters like that. At least I remember having such
> cases in the past. If I am right, then there is quite a chance of
> invalidating working code.


In terms of the JavaBeans specification, the setters must be void. If you
change the return type, it is no longer a JavaBeans setter, and many tools
will not see it. However, the above mentioned suggestion does not require a
change of the return type, so the bean methods and all their signatures
will be 100% backwards compatible.

With this suggested change, the only behavior that will change is that some
code which used to not compile will start compiling, with a reasonable
result. No code that used to compile will change.

Re: JDK-8038451: Incorrect initialization order of static fields in sun.nio.cs.ext.JISAutoDetect$Decoder

[Xueming Shen]

On 03/27/2014 01:49 PM, Mandy Chung wrote:

This fixes the regression introduced by JDK-8038177 in which the osName field 
should be initialized before it's accessed in the class static initializer.

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

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8038451/webrev.00/

Thanks
Mandy

looks fine.

Re: Implicit 'this' return for void methods

[Jochen Theodorou]

Am 27.03.2014 21:52, schrieb Eirik Lygre:
[...]

The JavaBean specification, with it's "void setSomething()" functions
are fundamental to so many things Java that they will never go away
(good thing, too!).The suggested language change builds on top of that,
is beneficial to a large body of existing code and does not invalidate
any existing practices.

It is, I think, a great example of bang for the buck -- low cost, high
return. No changes needed in the VM; everything can be done by the
compiler. Almost too sweet.


Hmm... isn't it a problem if I change "void setSomething()" to "MyClass 
setSomething()". In example, is that still a valid setter in terms of 
the JavaBean Spec? I might be wrong, but afaik there are bean based 
tools out there, not recognizing setters like that. At least I remember 
having such cases in the past. If I am right, then there is quite a 
chance of invalidating working code.



I should have paid more attention to the dragon book, so that I could
have taken a shot at the compiler myself :-/


I think you won't need it for that kind of change. You don't change the 
grammar, or any first level rules. You would change the internal AST, or 
even the bytecode generation part for this. You need to know javac 
internals instead... bad enough imho


bye blackdrag

--
Jochen "blackdrag" Theodorou - Groovy Project Tech Lead
blog: http://blackdragsview.blogspot.com/
german groovy discussion newsgroup: de.comp.lang.misc
For Groovy programming sources visit http://groovy-lang.org

Re: JDK-8038451: Incorrect initialization order of static fields in sun.nio.cs.ext.JISAutoDetect$Decoder

[Alan Bateman]

On 27/03/2014 20:49, Mandy Chung wrote:
This fixes the regression introduced by JDK-8038177 in which the 
osName field should be initialized before it's accessed in the class 
static initializer.


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

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8038451/webrev.00/

Easily done, moving it to the top looks okay to me.

-Alan.

Re: Implicit 'this' return for void methods

[Eirik Lygre]

On Thu, Mar 27, 2014 at 5:35 PM, Jochen Theodorou  wrote:

> Am 26.03.2014 16:51, schrieb Guy Steele:
> [...]
>
>  I am wholeheartedly in favor of allowing "chaining" of dotted expressions
>> such as
>>
>> CharBuffer.allocate(26).position(2).put("C").position(25).put("Z")
>>
>
> this also shows a potential point of critic this proposal will have to
> find arguments against. This style highly encourages mutation of the
> object, which is not very functional. Since java8 java tries to be more
> functional, thus this idea would be totally against this new line.
>

The JavaBean specification, with it's "void setSomething()" functions are
fundamental to so many things Java that they will never go away (good
thing, too!).The suggested language change builds on top of that, is
beneficial to a large body of existing code and does not invalidate any
existing practices.

It is, I think, a great example of bang for the buck -- low cost, high
return. No changes needed in the VM; everything can be done by the
compiler. Almost too sweet.

I should have paid more attention to the dragon book, so that I could have
taken a shot at the compiler myself :-/

Eirik

JDK-8038451: Incorrect initialization order of static fields in sun.nio.cs.ext.JISAutoDetect$Decoder

[Mandy Chung]

This fixes the regression introduced by JDK-8038177 in which the osName 
field should be initialized before it's accessed in the class static 
initializer.


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

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8038451/webrev.00/

Thanks
Mandy

Re: Implicit 'this' return for void methods

[Jochen Theodorou]

Am 27.03.2014 20:22, schrieb Steven Schlansker:


On Mar 27, 2014, at 9:35 AM, Jochen Theodorou  wrote:


Am 26.03.2014 16:51, schrieb Guy Steele:
[...]

I am wholeheartedly in favor of allowing “chaining” of dotted expressions such 
as

CharBuffer.allocate(26).position(2).put("C").position(25).put("Z”)


this also shows a potential point of critic this proposal will have to find 
arguments against. This style highly encourages mutation of the object, which 
is not very functional. Since java8 java tries to be more functional, thus this 
idea would be totally against this new line.

I leave it to you guys if this is even a valid argument ;)


It would be much easier to convince people to do less mutation if the language 
had support for the Builder pattern or freezing objects.


I wanted to counter that people would probably complain about the huge 
amount of objects you create, but actually you can do this with a helper 
class producing a dummy, that then becomes the final object of the real 
class once you freeze or otherwise fixate it. Well, the details don't 
really matter. But yes, I agree... some language support here would 
help, though not sure how that would look like...



Currently working with nontrivial immutable objects quickly leads to a lot of 
boilerplate code that requires frustrating maintenance every time the object 
definition changes.


yeah, with the approach I sketched I can totally see that.


Adding both the implicit ‘this’ proposal as well as some syntax enhancements 
for immutable objects would strengthen the Java language quite a bit, in my 
opinion :)


And here I fail. If you need special support for the builder and freezer 
to produce some helper or something already, then you don't need the 
return 'this' for void at all. The helper would be generated and thus 
already have all the bits you want. Right now I am actually wondering if 
I should add that as a transform to Groovy. But well, this is Java.


For the part with immutable I agree again of course.

byeJochen

--
Jochen "blackdrag" Theodorou - Groovy Project Tech Lead
blog: http://blackdragsview.blogspot.com/
german groovy discussion newsgroup: de.comp.lang.misc
For Groovy programming sources visit http://groovy-lang.org

Re: Implicit 'this' return for void methods

[Remi Forax]

On 03/27/2014 08:22 PM, Steven Schlansker wrote:

On Mar 27, 2014, at 9:35 AM, Jochen Theodorou  wrote:


Am 26.03.2014 16:51, schrieb Guy Steele:
[...]

I am wholeheartedly in favor of allowing “chaining” of dotted expressions such 
as

CharBuffer.allocate(26).position(2).put("C").position(25).put("Z”)

this also shows a potential point of critic this proposal will have to find 
arguments against. This style highly encourages mutation of the object, which 
is not very functional. Since java8 java tries to be more functional, thus this 
idea would be totally against this new line.

I leave it to you guys if this is even a valid argument ;)

It would be much easier to convince people to do less mutation if the language 
had support for the Builder pattern or freezing objects.  Currently working 
with nontrivial immutable objects quickly leads to a lot of boilerplate code 
that requires frustrating maintenance every time the object definition changes.

Adding both the implicit ‘this’ proposal as well as some syntax enhancements 
for immutable objects would strengthen the Java language quite a bit, in my 
opinion :)


I fully agree.

Rémi

Re: RFR(S) : 8038186 : [TESTBUG] improvements of test j.l.i.MethodHandles

[Igor Ignatyev]

After an offline conversation w/ VladimirI, I've done several changes:
- placed all classes into one file (but I don't like it)
- changed package name
- extracted creating tests code from test execution code

I hope the code's still quite readable and understandable.

Also I've added array return type.

http://cr.openjdk.java.net/~iignatyev/8038186/webrev.02/

Thanks,
Igor

On 03/27/2014 12:46 AM, Igor Ignatyev wrote:

Hi Chris,


This is very nice.  Are there plans to do this for the other existing
tests as well?

sure, but we didn't have enough time to do it in 8u20 timeframe. So I'd
like to push these changes 1st, since we need this to cover 'Speedup
catch exceptions'. Other tests will be rewrite later. I hope we'll have
time in jdk 9 to refactor all tests in MethodHandles.

Thanks,
Igor

On 03/27/2014 12:38 AM, Christian Thalinger wrote:


On Mar 24, 2014, at 1:33 PM, Igor Ignatyev 
wrote:


Hi all,

Please review the patch:

Problems:
- MethodHandlesTest::testCatchException() doesn't provide enough
testing of j.l.i.MethodHandles::catchException().
- MethodHandlesTest contains more than 3k lines, an auxiliary code
together w/ a test code, many methods aren't connected w/ each other,
etc. All this leads to that it is very very hard to understand,
maintain.


Yes, it is.



Fix:
- the auxiliary code was moved to testlibray
- the test code was moved to a separate subpackage


This is very nice.  Are there plans to do this for the other existing
tests as well?


- random signature is used for target and handler
- added scenarios w/ different return types

webrev: http://cr.openjdk.java.net/~iignatyev/8038186/webrev.01/
jbs: https://bugs.openjdk.java.net/browse/JDK-8038186
--
Igor
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev


___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev


___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: Implicit 'this' return for void methods

[Steven Schlansker]

On Mar 27, 2014, at 9:35 AM, Jochen Theodorou  wrote:

> Am 26.03.2014 16:51, schrieb Guy Steele:
> [...]
>> I am wholeheartedly in favor of allowing “chaining” of dotted expressions 
>> such as
>> 
>>CharBuffer.allocate(26).position(2).put("C").position(25).put("Z”)
> 
> this also shows a potential point of critic this proposal will have to find 
> arguments against. This style highly encourages mutation of the object, which 
> is not very functional. Since java8 java tries to be more functional, thus 
> this idea would be totally against this new line.
> 
> I leave it to you guys if this is even a valid argument ;)

It would be much easier to convince people to do less mutation if the language 
had support for the Builder pattern or freezing objects.  Currently working 
with nontrivial immutable objects quickly leads to a lot of boilerplate code 
that requires frustrating maintenance every time the object definition changes.

Adding both the implicit ‘this’ proposal as well as some syntax enhancements 
for immutable objects would strengthen the Java language quite a bit, in my 
opinion :)

Re: RFR(S): 8038233 : Fix unsafe strcpy in Java_sun_tools_attach_{Aix, Bsd, Linux}VirtualMachine_connect()

[Alan Bateman]

On 27/03/2014 18:08, Volker Simonis wrote:

Hi,

a security audit for the PPC64/AIX port revealed an unsecure useage of
'strcpy' in Java_sun_tools_attach_AixVirtualMachine_connect(). Because
the same coding is also used in the Linux and BSD implementations, the
following change fixes them all together:

http://cr.openjdk.java.net/~simonis/webrevs/8038233/
https://bugs.openjdk.java.net/browse/JDK-8038233

Compiled and tested (with the com/sun/jdi, com/sun/tools/attach,
com/sun/management and sun/management JTreg tests) on Linux, MacOS X
and AIX.

Please notice that this fix is also intended for backporting tu 8u.

As we now have 3 implementations using socket pairs then there might be 
an opportunity to consolidate the implementations, not for this patch of 
course.


Your changes looks okay and it's good to memset the sockaddr_un 
structure. As regards the issue is a concern or not then it's important 
to note thaht the Attach API is in tools.jar, it's not in a JRE build. 
Also an attach requires a Permission check if running with a security 
manager and I likely vert rare for tools to do this. Also to cause a 
problem then it would require the temporary directory to be unusually long.


-Alan.

RFR(S): 8038233 : Fix unsafe strcpy in Java_sun_tools_attach_{Aix, Bsd, Linux}VirtualMachine_connect()

[Volker Simonis]

Hi,

a security audit for the PPC64/AIX port revealed an unsecure useage of
'strcpy' in Java_sun_tools_attach_AixVirtualMachine_connect(). Because
the same coding is also used in the Linux and BSD implementations, the
following change fixes them all together:

http://cr.openjdk.java.net/~simonis/webrevs/8038233/
https://bugs.openjdk.java.net/browse/JDK-8038233

Compiled and tested (with the com/sun/jdi, com/sun/tools/attach,
com/sun/management and sun/management JTreg tests) on Linux, MacOS X
and AIX.

Please notice that this fix is also intended for backporting tu 8u.

Thank you and best regards,
Volker

Re: Implicit 'this' return for void methods

[Jochen Theodorou]

Am 26.03.2014 16:51, schrieb Guy Steele:
[...]

I am wholeheartedly in favor of allowing “chaining” of dotted expressions such 
as

CharBuffer.allocate(26).position(2).put("C").position(25).put("Z”)


this also shows a potential point of critic this proposal will have to 
find arguments against. This style highly encourages mutation of the 
object, which is not very functional. Since java8 java tries to be more 
functional, thus this idea would be totally against this new line.


I leave it to you guys if this is even a valid argument ;)

bye Jochen

--
Jochen "blackdrag" Theodorou - Groovy Project Tech Lead
blog: http://blackdragsview.blogspot.com/
german groovy discussion newsgroup: de.comp.lang.misc
For Groovy programming sources visit http://groovy-lang.org

Re: Implicit 'this' return for void methods

[Ulf Zibis]

Hi Guy and Paul,

thanks for liking my proposal.

What can we do to convince the "officials" ?

-Ulf


Am 26.03.2014 17:20, schrieb Paul Benedict:
It would be nice to make this language change. In the past years, it's pretty clear many JSR EE 
spec leads have gone on to make their APIs return the same object because they strongly prefer to 
see object chaining in code. I sympathize with those designers, but I don't agree; I wouldn't 
affect my API just for the sake of chaining. For the sake of clarity, I prefer functions that 
don't compute anything to return void. So +1 for the language to figure this out.



On Wed, Mar 26, 2014 at 10:51 AM, Guy Steele > wrote:



On Mar 26, 2014, at 4:17 AM, Ulf Zibis  wrote:

> See also:
> . . .
> http://mail.openjdk.java.net/pipermail/coin-dev/2009-March/001180.html

This last one has a specific proposal, which is simple and quite nice.  The 
important idea is
that we don't actually make any change to the code of void methods or make 
them actually
return anything; instead, the caller takes notice of situations where an 
invocation of
a void method is used as a subexpression whose value is required 
(heretofore forbidden
by the language) and gives it a special interpretation.

I note that Ulf's proposal applies only to method invocations, but I note 
that the same
technique could be used to include field references if desired.

I am wholeheartedly in favor of allowing "chaining" of dotted expressions 
such as

 CharBuffer.allocate(26).position(2).put("C").position(25).put("Z")

I am a bit more skeptical about expressions that begin with a dot because 
of potential
confusion about which expression is referred to:

myVeryLongNamedString.subString(.indexOf("C"), .indexOf("Q"))

seems clear enough, but what about:

myVeryLongNamedString.subString(.indexOf("C") + otherString.length(), 
.indexOf("Q"))

Does the second occurrence of .indexOf use myVeryLongNamedString or 
otherString?

A compromise would be to allow leading-dot expressions to occur only within 
the arguments
of the method call whose target is the object which the leading-dot 
expressions are expected
to use as their target, and if there are such leading-dot expressions 
within the arguments
then the arguments must not contain any non-leading-dot field references or 
method calls.
Just a thought for discussion.  This would be considered a separate 
mechanism from the
chaining-of-void-methods mechanism (it was a very clever idea to try to 
unify them in Ulf's
original proposal, though).

---Guy




--
Cheers,
Paul

Re: Improve timezone mapping for AIX platform

[Masayoshi Okutsu]

Hi Volker,

You are right. The `tz' value needs to be given to 
getPlatformTimeZoneID() or the getenv() call should be moved to the 
function. Your mapPlatformToJavaTimzone(const char* tz) sounds good to me.


In any case TimeZone_md.c has become too messy by accumulating 
platform/release-specific code. I guess it's time to clean up...


Thanks,
Masayoshi

On 3/27/2014 2:48 AM, Volker Simonis wrote:

Hi Jonathan,

thanks for doing this change. Please find some comments below:

1. please update the copyright year to 2014 in every file you touch

2. in CopyFiles.gmk you can simplify the change by joining the windows
and aix cases because on Windows OPENJDK_TARGET_OS is the same like
OPENJDK_TARGET_OS_API_DIR. So you can write:

ifneq ($(findstring $(OPENJDK_TARGET_OS), windows aix), )

   TZMAPPINGS_SRC := $(JDK_TOPDIR)/src/$(OPENJDK_TARGET_OS)/lib

   $(LIBDIR)/tzmappings: $(TZMAPPINGS_SRC)/tzmappings
 $(call install-file)

   COPY_FILES += $(LIBDIR)/tzmappings

endif

3. regarding the changes proposed by Masayoshi: I agree that we should
put the AIX timezone mapping code in its own function, but I don't
think that getPlatformTimeZoneID() would be the right place. As far as
I understand, getPlatformTimeZoneID() is used to get a platform time
zone id if the environment variable "TZ" is not set. I don't know a
way how this could be done on AIX (@Jonathan: maybe you know?). If
there would be a way to get the time zone from some system files or
so, then we should put this code into the AIX version of
getPlatformTimeZoneID().

But the current AIX code contributed by Jonathan, actually uses the
time zone id from the "TZ" environment variable and just maps it to a
Java compatible time zone id. So I'd suggest to refactor this code
into a function like for example "static const char*
mapPlatformToJavaTimzone(const char* tz)" and call that from
findJavaTZ_md(). I think that would make the code easier to
understand.

@Masayoshi: what do you think, would you agree with such a solution.

4. I agree with Masayoshi that you should use the existing getGMTOffsetID()

5. Please notice that your change breaks all Unix builds except AIX because of:

  759 }
  760 tzerr:
  761 if (javatz == NULL) {
  762 time_t offset;
...
  782 }
  783 #endif

I think that should read:

  759
  760 tzerr:
  761 if (javatz == NULL) {
  762 time_t offset;
...
  782 }
  783 #endif
  784 }

Refactoring the code in an extra function will make that error more
evident anyway.

But please always build at least on one different platform (i.e.
Linux) to prevent such errors in the future.

Regards,
Volker


On Wed, Mar 26, 2014 at 10:27 AM, Masayoshi Okutsu
 wrote:

Hi Jonathan,

The AIX specific code looks OK to me. But I'd suggest the code be moved to
getPlatformTimeZoneID() for AIX, which just returns NULL currently. Also
there's a function for producing a fallback ID in "GMT±hh:mm",
getGMTOffsetID() which can be called in tzerr.

Thanks,
Masayoshi


On 3/26/2014 3:47 PM, Jonathan Lu wrote:

Hi ppc-aix-port-dev, core-libs-dev,

Here's a patch for JDK-8034220,

http://cr.openjdk.java.net/~luchsh/JDK-8034220/

It is trying to add the a more complete timezone mapping mechanism for AIX
platform.
the changes are primarily based on IBM's commercial JDK code, which
includes:

- A new timezone mapping file added under directory jdk/src/aix/lib/
- Code to parse above config file, changed file is
src/solaris/native/java/util/TimeZone_md.c
- And also makefile change in make/CopyFiles.gmk to copy the config file

Could you pls help to review and give comments ?

Cheers
- Jonathan

Re: RFR [8038333] java/lang/ref/EarlyTimeout.java failed

[Ivan Gerasimov]

On 27.03.2014 18:00, Peter Levart wrote:

On 03/27/2014 02:36 PM, Ivan Gerasimov wrote:

Thank you Peter!

There could be a little delay (say half the timeout: 500ms) 
specified after main thread returns from the startedSignal.await(); 
and before setting referent = null; and doing System.gc(). This 
would decrease the chance that the reference is enqueued before 
EarlyTimeout threads enter queue.remove(1000), thus making the test 
more reliable in failing with unpatched code.



Yes, you're right.
I've checked the test against jdk9-b01 (no fix for 6853696 yet).
It gives 2.5% of false negatives (i.e. in 5 out of 200 runs the 
reference was enqueued before calling to remove()).

With an additional delay of TIMEOUT / 2 this number dropped to 0%.

With jdk9-b05 (with fix for 6853696) no false positives were shown 
with or without this additional delay.


Now even if the referent is released and System.gc() is called, that 
does not guarantee that a WeakReference is going to be enqueued 
before the EarlyTimeout threads timeout and the result could as well 
be 0 collected references. To increase the chance that the reference 
is enqueued in a timely manner, main thread could, immediately after 
System.gc(), call:


SharedSecrets.getJavaLangRefAccess().tryHandlePendingReference();

(since SharedSecrets is in sun.misc protected package, 
JavaLangRefAccess instance would have to be obtained using 
reflection unfortunately).




I would prefer not to complicate the test too much, if you don't 
mind. I think the test already shows reliable reproducibility.



I suggest to return to the very first trivial fix:
http://cr.openjdk.java.net/~igerasim/8038333/1/webrev/


But this webrev *is moving startedSignal.await() after System.gc()* ...



Oops, sorry. It was meant to be /8038333/*0*/webrev/, of course!

Now, I updated the webrev with the additional delay as you suggested:
http://cr.openjdk.java.net/~igerasim/8038333/2/webrev/

Would you please have a look?


That looks good. So no warning if nonNullRefCount == 0 ... It is 
probably not particularly useful if nobody is going to look at it, right?



Yeah.
I think that the test will only bring attention if it fails.



Regards, Peter



Sincerely yours,
Ivan

Re: RFR [8038333] java/lang/ref/EarlyTimeout.java failed

[Peter Levart]

On 03/27/2014 02:36 PM, Ivan Gerasimov wrote:

Thank you Peter!

There could be a little delay (say half the timeout: 500ms) specified 
after main thread returns from the startedSignal.await(); and before 
setting referent = null; and doing System.gc(). This would decrease 
the chance that the reference is enqueued before EarlyTimeout threads 
enter queue.remove(1000), thus making the test more reliable in 
failing with unpatched code.



Yes, you're right.
I've checked the test against jdk9-b01 (no fix for 6853696 yet).
It gives 2.5% of false negatives (i.e. in 5 out of 200 runs the 
reference was enqueued before calling to remove()).

With an additional delay of TIMEOUT / 2 this number dropped to 0%.

With jdk9-b05 (with fix for 6853696) no false positives were shown 
with or without this additional delay.


Now even if the referent is released and System.gc() is called, that 
does not guarantee that a WeakReference is going to be enqueued 
before the EarlyTimeout threads timeout and the result could as well 
be 0 collected references. To increase the chance that the reference 
is enqueued in a timely manner, main thread could, immediately after 
System.gc(), call:


SharedSecrets.getJavaLangRefAccess().tryHandlePendingReference();

(since SharedSecrets is in sun.misc protected package, 
JavaLangRefAccess instance would have to be obtained using reflection 
unfortunately).




I would prefer not to complicate the test too much, if you don't mind. 
I think the test already shows reliable reproducibility.



I suggest to return to the very first trivial fix:
http://cr.openjdk.java.net/~igerasim/8038333/1/webrev/


But this webrev *is moving startedSignal.await() after System.gc()* ...



Oops, sorry. It was meant to be /8038333/*0*/webrev/, of course!

Now, I updated the webrev with the additional delay as you suggested:
http://cr.openjdk.java.net/~igerasim/8038333/2/webrev/

Would you please have a look?


That looks good. So no warning if nonNullRefCount == 0 ... It is 
probably not particularly useful if nobody is going to look at it, right?


Regards, Peter



Sincerely yours,
Ivan

Re: RFR [8038333] java/lang/ref/EarlyTimeout.java failed

[Ivan Gerasimov]

Thank you Peter!

There could be a little delay (say half the timeout: 500ms) specified 
after main thread returns from the startedSignal.await(); and before 
setting referent = null; and doing System.gc(). This would decrease 
the chance that the reference is enqueued before EarlyTimeout threads 
enter queue.remove(1000), thus making the test more reliable in 
failing with unpatched code.



Yes, you're right.
I've checked the test against jdk9-b01 (no fix for 6853696 yet).
It gives 2.5% of false negatives (i.e. in 5 out of 200 runs the 
reference was enqueued before calling to remove()).

With an additional delay of TIMEOUT / 2 this number dropped to 0%.

With jdk9-b05 (with fix for 6853696) no false positives were shown with 
or without this additional delay.


Now even if the referent is released and System.gc() is called, that 
does not guarantee that a WeakReference is going to be enqueued before 
the EarlyTimeout threads timeout and the result could as well be 0 
collected references. To increase the chance that the reference is 
enqueued in a timely manner, main thread could, immediately after 
System.gc(), call:


SharedSecrets.getJavaLangRefAccess().tryHandlePendingReference();

(since SharedSecrets is in sun.misc protected package, 
JavaLangRefAccess instance would have to be obtained using reflection 
unfortunately).




I would prefer not to complicate the test too much, if you don't mind. I 
think the test already shows reliable reproducibility.



I suggest to return to the very first trivial fix:
http://cr.openjdk.java.net/~igerasim/8038333/1/webrev/


But this webrev *is moving startedSignal.await() after System.gc()* ...



Oops, sorry. It was meant to be /8038333/*0*/webrev/, of course!

Now, I updated the webrev with the additional delay as you suggested:
http://cr.openjdk.java.net/~igerasim/8038333/2/webrev/

Would you please have a look?

Sincerely yours,
Ivan

Re: RFR [8038333] java/lang/ref/EarlyTimeout.java failed

[Peter Levart]

Hi Ivan,

On 03/27/2014 08:26 AM, Ivan Gerasimov wrote:

David, Mandy, thank you for comments!

Here's what we want to achieve in the test:
Two EarlyTimeout child threads should both be blocked in 
remove(TIMEOUT) at the moment the weakReference gets enqueued.

This is the situation, when the bug 6853696 manifests itself.

If we made sure the weakReference is enqueued before remove() is 
called by the child threads, then they both would wait for specified 
amount of time even without the fix.


That's why we make the main thread wait for the children to start 
before calling System.gc().

I understand it may seem strange at the first glance.
But we want the child thread to be inside the remove() function when 
the main thread is unblocked, that's why it is done this way.


Unfortunately, we cannot easily make sure that remove() was called by 
the child threads at the moment the GC decides to enqueue the 
weakReference.


There could be a little delay (say half the timeout: 500ms) specified 
after main thread returns from the startedSignal.await(); and before 
setting referent = null; and doing System.gc(). This would decrease the 
chance that the reference is enqueued before EarlyTimeout threads enter 
queue.remove(1000), thus making the test more reliable in failing with 
unpatched code.


Now even if the referent is released and System.gc() is called, that 
does not guarantee that a WeakReference is going to be enqueued before 
the EarlyTimeout threads timeout and the result could as well be 0 
collected references. To increase the chance that the reference is 
enqueued in a timely manner, main thread could, immediately after 
System.gc(), call:


SharedSecrets.getJavaLangRefAccess().tryHandlePendingReference();

(since SharedSecrets is in sun.misc protected package, JavaLangRefAccess 
instance would have to be obtained using reflection unfortunately).


Therefore, the test is probabilistic by its nature, and we cannot be 
absolutely sure that at least one child thread will get non-null 
reference returned from remove().


I'm withdrawing the second webrev: While moving startedSignal.await() 
after System.gc() reduces the number of cases when nonNullRefCount == 
0, it also greatly decreases the chances of occurring the situation 
the test was designed for.


I believe the correct fix here is to ignore the (quite rare) situation 
when nonNullRefCount upon the completion.

I suggest to return to the very first trivial fix:
http://cr.openjdk.java.net/~igerasim/8038333/1/webrev/


But this webrev *is moving startedSignal.await() after System.gc()* ...

Regards, Peter



Sincerely yours,
Ivan

On 27.03.2014 4:30, Mandy Chung wrote:

On 3/26/2014 3:57 PM, David Holmes wrote:

Ivan,

I think the problem is that the EarlyTimeout threads can complete 
their remove(TIMEOUT) before the main thread has started them all, 
cleared the reference and called System.gc().


Depending on exactly what is being tested, the EarlyTimeout threads 
may need to wait on another latch that is signalled by the main 
thread after the gc() call returns. Still no guarantee that the gc 
will do its work before the timeout elapses.




This is similar to what I have been thinking.  I believe the 
EarlyTimeout threads don't need the startedSignal.countDown; instead 
it can have a signal latch with 1 count.  The EarlyTimeout threads 
awaits on the signal latch.  The main thread will call 
signal.countDown after System.gc() and we can add a check 
weakReference.isEnqueued to make sure before we awake the threads to 
proceed the queue.remove call.


Ivan - for the error message, perhaps you can simply do this:

  if (nonNullRefCount != 1) {
   throw new RuntimeException(nonNullRefCount + " references 
were removed from queue");

  }


Mandy


David

On 27/03/2014 6:18 AM, Ivan Gerasimov wrote:

Thank you Mandy!


Are you able to reproduce the test failure?


Yes, I could easily reproduce the failure when I reduced the 
timeout to

10 ms.
With the timeout reduced, the test fails every third time on my 
machine.


I think the test verifies that only one thread gets the reference 
is a

good test.

But if none of the threads could get the reference, it should not 
cause

the failure of the test.
It only means that during this particular run we could not have tested
what we needed.
We could retry, but I'm not sure it's worth complicating the test.
It's easier to ignore the failure, taking into account that it happens
rarely.


I think the race is due to the threads get to call queue.remove as
soon as both threads decrement the count of the latch that can be 
well

before the reference is enqueued.

The problem is that we have no way to block the main thread until 
there

is a reference in the queue.
I improved the situation a bit, having moved the await() after the 
call

of gc().


It'd be good to add additional information in the test to help
diagnosing test failure.


I added reporting to stderr about being unable to remove a

Re: RFR: 8000975: (process) Merge UNIXProcess.java.bsd & UNIXProcess.java.linux (& .solaris & .aix)

[Volker Simonis]

Hi Peter,

thanks for applying these changes to the AIX files as well.

With the additional line:

if (osName.equals("AIX")) { return AIX; }

in Os.get() your change compiles cleanly on AIX and runs the
java/lang/ProcessBuilder tests without any errors.

So from an AIX perspective, thumbs up.

Regards,
Volker


On Wed, Mar 26, 2014 at 5:18 PM, Alan Bateman  wrote:
> On 26/03/2014 15:19, Peter Levart wrote:
>>
>> I couldn't find any official document about possible os.name values for
>> different supported OSes. Does anyone have a pointer?
>
> I don't know if there is a definite list but I assume we don't need to be
> concerned with anything beyond the 4 that we have in OpenJDK, which is
> "Linux", "SunOS", "AIX" and contains("OS X").
>
> If we get to the point in JDK 9 where src/solaris is renamed to src/unix (or
> something equivalent) then it could mean that the Os enum can be replaced
> with an OS specific class in src/linux, src/solaris, ... and this would
> avoid the need for an os.name check at runtime.
>
> -Alan.
>

Re: RFR [8038333] java/lang/ref/EarlyTimeout.java failed

[Ivan Gerasimov]

David, Mandy, thank you for comments!

Here's what we want to achieve in the test:
Two EarlyTimeout child threads should both be blocked in remove(TIMEOUT) 
at the moment the weakReference gets enqueued.

This is the situation, when the bug 6853696 manifests itself.

If we made sure the weakReference is enqueued before remove() is called 
by the child threads, then they both would wait for specified amount of 
time even without the fix.


That's why we make the main thread wait for the children to start before 
calling System.gc().

I understand it may seem strange at the first glance.
But we want the child thread to be inside the remove() function when the 
main thread is unblocked, that's why it is done this way.


Unfortunately, we cannot easily make sure that remove() was called by 
the child threads at the moment the GC decides to enqueue the weakReference.
Therefore, the test is probabilistic by its nature, and we cannot be 
absolutely sure that at least one child thread will get non-null 
reference returned from remove().


I'm withdrawing the second webrev: While moving startedSignal.await() 
after System.gc() reduces the number of cases when nonNullRefCount == 0, 
it also greatly decreases the chances of occurring the situation the 
test was designed for.


I believe the correct fix here is to ignore the (quite rare) situation 
when nonNullRefCount upon the completion.

I suggest to return to the very first trivial fix:
http://cr.openjdk.java.net/~igerasim/8038333/1/webrev/

Sincerely yours,
Ivan

On 27.03.2014 4:30, Mandy Chung wrote:

On 3/26/2014 3:57 PM, David Holmes wrote:

Ivan,

I think the problem is that the EarlyTimeout threads can complete 
their remove(TIMEOUT) before the main thread has started them all, 
cleared the reference and called System.gc().


Depending on exactly what is being tested, the EarlyTimeout threads 
may need to wait on another latch that is signalled by the main 
thread after the gc() call returns. Still no guarantee that the gc 
will do its work before the timeout elapses.




This is similar to what I have been thinking.  I believe the 
EarlyTimeout threads don't need the startedSignal.countDown; instead 
it can have a signal latch with 1 count.  The EarlyTimeout threads 
awaits on the signal latch.  The main thread will call 
signal.countDown after System.gc() and we can add a check 
weakReference.isEnqueued to make sure before we awake the threads to 
proceed the queue.remove call.


Ivan - for the error message, perhaps you can simply do this:

  if (nonNullRefCount != 1) {
   throw new RuntimeException(nonNullRefCount + " references 
were removed from queue");

  }


Mandy


David

On 27/03/2014 6:18 AM, Ivan Gerasimov wrote:

Thank you Mandy!


Are you able to reproduce the test failure?


Yes, I could easily reproduce the failure when I reduced the timeout to
10 ms.
With the timeout reduced, the test fails every third time on my 
machine.



I think the test verifies that only one thread gets the reference is a
good test.


But if none of the threads could get the reference, it should not cause
the failure of the test.
It only means that during this particular run we could not have tested
what we needed.
We could retry, but I'm not sure it's worth complicating the test.
It's easier to ignore the failure, taking into account that it happens
rarely.


I think the race is due to the threads get to call queue.remove as
soon as both threads decrement the count of the latch that can be well
before the reference is enqueued.


The problem is that we have no way to block the main thread until there
is a reference in the queue.
I improved the situation a bit, having moved the await() after the call
of gc().


It'd be good to add additional information in the test to help
diagnosing test failure.


I added reporting to stderr about being unable to remove a reference
from the queue.
I believe we shouldn't treat it as an error, and should simply 
ignore it.


Would you please have a look at the updated webrev:
http://cr.openjdk.java.net/~igerasim/8038333/1/webrev/

Sincerely yours,
Ivan