RE: RFR (JAXP): 8146961: Fix PermGen memory leaks caused by static final Exceptions

[Langer, Christoph]

+1, nice fix.

> -Original Message-
> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf
> Of Joe Wang
> Sent: Mittwoch, 17. August 2016 18:30
> To: Aleks Efimov 
> Cc: core-libs-dev 
> Subject: Re: RFR (JAXP): 8146961: Fix PermGen memory leaks caused by static
> final Exceptions
> 
> Looks good, Aleksej. Thanks for coming up with a well-thought solution
> to get rid of the static RE field.
> 
> Best,
> Joe
> 
> On 8/17/16, 7:04 AM, Aleks Efimov wrote:
> > Hi Joe, Aleksey,
> > Thank you for reviewing the initial fix.
> >
> > I followed the Joe's suggestion (thanks for that) and removed static
> > 'abort' field completely, the functionality was replaced by throwing
> > the exception of newly added type - AbortException. The new webrev
> > with removed 'abort' can be found here:
> > http://cr.openjdk.java.net/~aefimov/8146961/9/01
> >
> > The Tomcat reproducer attached to the bug report fails without the fix
> > and passes with the new version of the fix.
> > The JPRT and JCK testing again shows no related jaxp tests failures.
> >
> > Best Regards,
> > Aleksej
> >
> >
> > On 15/08/16 21:09, Joe Wang wrote:
> >> Hi Aleksej,
> >>
> >> I suggest we get rid of the static abort. If RuntimeException
> >> happens, check where it happens. The first use case looks suspicious
> >> as it just returns if it's an instance of RE. For the 2nd case in DOM
> >> error report, let's throw a RuntimeException with the specified error
> >> message if error happens, and there's no handler or the handler
> >> failed to handle it. (would be better than an empty RE)
> >>
> >> Best,
> >> Joe
> >>
> >> On 8/15/16, 10:38 AM, Aleks Efimov wrote:
> >>> Hi,
> >>>
> >>> Please, help to review the fix for memory leak [1] in
> >>> com.sun.org.apache.xerces.internal.dom.DOMNormalizer that is caused
> >>> by usage of static final exceptions.
> >>> This problem was already fixed in Apache Xerces project [2] and I
> >>> would like to backport it to JDK9 Xerces implementation.
> >>> Webrev with the changes:
> >>> http://cr.openjdk.java.net/~aefimov/8146961/9/00
> >>>
> >>> The Tomcat reproducer attached to the bug report fails without the
> >>> fix and passes with the fix.
> >>> The JPRT and JCK testing shows no jaxp tests failures with the
> >>> proposed fix.
> >>>
> >>> With Best Regards,
> >>> Aleksej
> >>>
> >>> [1] https://bugs.openjdk.java.net/browse/JDK-8146961
> >>> [2] https://issues.apache.org/jira/browse/XERCESJ-1667
> >>>
> >

Fork/Join exceptional completion can return before all tasks have finished executing Re: Exceptional behavior of parallel stream

[Paul Sandoz]

Hi Tagir,

Hmm… we certainly ensure that any short-circuiting computation will wait until 
all the computation is complete (tasks still running whose results will be 
thrown away have to complete before the terminal operation returns).

In that sprit we should be consistent when exceptions are thrown.

But we don’t currently do anything or say anything when a task exceptionally 
completes, which propagates exceptional completion status up through the 
computation tree, and when the root task is hit (whose status has the SIGNAL 
bit set) the caller thread will be notified. This happens independently of task 
completion via pending counts of CountedCompleter and rest of the computation 
continues on blissfully unaware.

I need to think about this a little more. To support this we may need to track 
exceptional completion explicitly by propagating state through CountedCompleter 
completions.

Paul.

RFR: 8164044: Generate corresponding simple DelegatingMethodHandles when generating a DirectMethodHandle

[Claes Redestad]

Hi,

please review this change which adds pre-generation of simple 
DelegatingMethodHandles corresponding to the DirectMethodHandles we 
already pre-generate during linking.


webrev: http://cr.openjdk.java.net/~redestad/8164044/webrev.02/
bug: https://bugs.openjdk.java.net/browse/JDK-8164044

This also includes some cleanup suggested by Vladimir during internal 
review, such as:


- adding an enum to control this behavior, which allows removing the 
special version of LF.compileToBytecode introduced by JDK-8163369

- refactored resolution of pregenerated code to the InvokerBytecodeGenerator
- removing reliance on the LF.debugName (which we have some loose ideas 
to remove from LF and tuck away in a map we only create and initialize 
when actually debugging).


This patch removes 11 out of the 39 classes generated on first use of 
the StringConcatFactory in a simple test, amounting to a ~10ms speedup 
on my machine.


Thanks!

/Claes

Re: RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect

[Frederic Parain]

Brent,

hotspot/src/share/vm/prims/stackwalk.cpp:
 230 if (!expressions &&
 231 values->at(i)->type() == T_INT &&
 232 values->int_at(i) == 0 && // empty first slot of long?
 233 i+1 < values->size() &&
 234 values->at(i+1)->type() == T_INT) {

How does this test make the difference between:
  1 - a local variable of type long
  2 - two consecutive local variables of type int with the value
  of the first one being zero

(1) requires the fix, but (2) does not.

Another question: is it guaranteed that an unused slot from
a pair-slot is always zero? This is not written in the JVM spec,
and I don't remember that the JVM zeroes unused slots.

A last question: the fix tries to provide a view of the local
variables that matches the JVM spec rather than the implementation,
so if half of the long or double value is put in the first slot,
why the second slot is not stripped to only store the other half?

Regards,

Fred

On 08/17/2016 04:05 PM, Brent Christian wrote:

Hi,

Please review this StackWalker fix in hotspot for 8156073, "2-slot
LiveStackFrame locals (long and double) are incorrect"

Bug: https://bugs.openjdk.java.net/browse/JDK-8156073
Webrev: http://cr.openjdk.java.net/~bchristi/8156073/webrev.01/

The experimental LiveStackFrame[1] interface for StackWalker provides
access to stack frames' local variables, returned in an Object[] from
LiveStackFrame.getLocals().

Long and double primitives are returned using two array slots (similar
to JVMS section 2.6.1 [2]).  This works as indicated on 32-bit JDKs, but
needs some fixing on 64-bit.

AFAICT under 64-bit, StackValueCollection stores the entire
long/double in the 2nd (64-bit) slot:

jlong StackValueCollection::long_at(int slot) const {
#ifdef _LP64
  return at(slot+1)->get_int();
#else
...

The first slot goes unused and contains 0, instead of holding half of
the long value as stackwalker expects.  So stackwalker needs to take
care splitting the value between the two slots.

The fix examines StackValueCollection::long_at(i), checks for an
unexpected 0 value from StackValueCollection::int_at(i), and copies
the needed 32-bits (depending on native byte ordering) into the first
slot as needed.  (The 2nd slot gets truncated to 32-bits in
create_primitive_value_instance()).

Here's the fix in action on linux_x64 with the updated
LocalsAndOperands.java test.  Slots 4+5 contain a long, 6+7 contain a
double.

Before the fix:
...
  local 3: himom type java.lang.String
  local 4: 0 type I  <--
  local 5: 65535 type I
  local 6: 0 type I  <--
  local 7: 1293080650 type I
  local 8: 0 type I

After the fix:
...
  local 3: himom type java.lang.String
  local 4: 1023 type I   <--
  local 5: 65535 type I
  local 6: 1074340347 type I <--
  local 7: 1293080650 type I
  local 8: 0 type I

This change also fixes 8158879, "Add new test for verifying 2-slot
locals in LiveStackFrame".  Thanks to Fabio Tudone
(fa...@paralleluniverse.co) for the test case.  I only test unused
("dead") local variables with -Xint, because they are removed in
compiled frames.

An automated build & test run on all platforms looks good.

It would be good to also do more testing of LiveStackFrame.getStack(),
but right now I want to focus on getting this getLocals() fix in.
Testing of LiveStackFrame.getStack() will happen in a follow-up issue
(8163825).

Thanks,
-Brent

1.
http://hg.openjdk.java.net/jdk9/hs/jdk/file/1efce54b06b7/src/java.base/share/classes/java/lang/LiveStackFrame.java

2.
https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-2.html#jvms-2.6.1

RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect

[Brent Christian]

Hi,

Please review this StackWalker fix in hotspot for 8156073, "2-slot 
LiveStackFrame locals (long and double) are incorrect"


Bug: https://bugs.openjdk.java.net/browse/JDK-8156073
Webrev: http://cr.openjdk.java.net/~bchristi/8156073/webrev.01/

The experimental LiveStackFrame[1] interface for StackWalker provides 
access to stack frames' local variables, returned in an Object[] from 
LiveStackFrame.getLocals().


Long and double primitives are returned using two array slots (similar 
to JVMS section 2.6.1 [2]).  This works as indicated on 32-bit JDKs, but 
needs some fixing on 64-bit.


AFAICT under 64-bit, StackValueCollection stores the entire
long/double in the 2nd (64-bit) slot:

jlong StackValueCollection::long_at(int slot) const {
#ifdef _LP64
  return at(slot+1)->get_int();
#else
...

The first slot goes unused and contains 0, instead of holding half of 
the long value as stackwalker expects.  So stackwalker needs to take 
care splitting the value between the two slots.


The fix examines StackValueCollection::long_at(i), checks for an 
unexpected 0 value from StackValueCollection::int_at(i), and copies
the needed 32-bits (depending on native byte ordering) into the first 
slot as needed.  (The 2nd slot gets truncated to 32-bits in 
create_primitive_value_instance()).


Here's the fix in action on linux_x64 with the updated 
LocalsAndOperands.java test.  Slots 4+5 contain a long, 6+7 contain a 
double.


Before the fix:
...
  local 3: himom type java.lang.String
  local 4: 0 type I  <--
  local 5: 65535 type I
  local 6: 0 type I  <--
  local 7: 1293080650 type I
  local 8: 0 type I

After the fix:
...
  local 3: himom type java.lang.String
  local 4: 1023 type I   <--
  local 5: 65535 type I
  local 6: 1074340347 type I <--
  local 7: 1293080650 type I
  local 8: 0 type I

This change also fixes 8158879, "Add new test for verifying 2-slot 
locals in LiveStackFrame".  Thanks to Fabio Tudone 
(fa...@paralleluniverse.co) for the test case.  I only test unused 
("dead") local variables with -Xint, because they are removed in 
compiled frames.


An automated build & test run on all platforms looks good.

It would be good to also do more testing of LiveStackFrame.getStack(), 
but right now I want to focus on getting this getLocals() fix in. 
Testing of LiveStackFrame.getStack() will happen in a follow-up issue 
(8163825).


Thanks,
-Brent

1. 
http://hg.openjdk.java.net/jdk9/hs/jdk/file/1efce54b06b7/src/java.base/share/classes/java/lang/LiveStackFrame.java

2. https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-2.html#jvms-2.6.1

Re: Collectors.toSet() small performance improvement

[Paul Sandoz]

> On 16 Aug 2016, at 20:08, Tagir F. Valeev  wrote:
> 
> Hello, Paul!
> 
> Thank you. Here's issue:
> https://bugs.openjdk.java.net/browse/JDK-8164189
> And webrev:
> http://cr.openjdk.java.net/~tvaleev/webrev/8164189/r1/
> 

Thanks will push today or tomorrow,
Paul.

> With best regards,
> Tagir Valeev.
> 
> PS> Hi Tagir,
> 
> PS> I like it. Please open an issue and i will sponsor the fix for you.
> 
> PS> Thanks,
> PS> Paul.
> 
>>> On 8 Aug 2016, at 22:31, Tagir F. Valeev  wrote:
>>> 
>>> Hello!
>>> 
>>> I'd like to propose a simple performance patch for Collectors.toSet():
>>> 
>>> --- a/src/java.base/share/classes/java/util/stream/Collectors.java  Tue 
>>> Aug 02 07:19:06 2016 +0530
>>> +++ b/src/java.base/share/classes/java/util/stream/Collectors.java  Tue 
>>> Aug 09 11:47:20 2016 +0700
>>> @@ -295,7 +295,12 @@
>>>public static 
>>>Collector toSet() {
>>>return new CollectorImpl<>((Supplier) HashSet::new, Set::add,
>>> -   (left, right) -> { left.addAll(right); 
>>> return left; },
>>> +   (left, right) -> {
>>> +   if (left.size() < right.size()) {
>>> +   right.addAll(left); return 
>>> right;
>>> +   }
>>> +   left.addAll(right); return left;
>>> +   },
>>>   CH_UNORDERED_ID);
>>>}
>>> 
>>> This will add the smaller set to the larger which may improve parallel
>>> performance when subtasks produced uneven results. In normal case the
>>> additional overhead is insignificant (additional HashSet size
>>> comparison per each combine operation which usually performed
>>> ~4*parallelism times).
>>> 
>>> Here's simple JDK benchmark with results (Core-i7-4702MQ 4 HT cores,
>>> Win7, JDK 9-ea+129 64bit) which confirms the speedup:
>>> http://cr.openjdk.java.net/~tvaleev/patches/toSet/
>>> 
>>> When data is unevenly distributed, it's significantly faster:
>>> 
>>> Original:
>>> ToSet.toSetParallel1  true  avgt   50 88,580 ±   0,874  
>>> us/op
>>> ToSet.toSetParallel   10  true  avgt   50676,243 ±  41,188  
>>> us/op
>>> ToSet.toSetParallel  100  true  avgt   50   9126,399 ±  83,260  
>>> us/op
>>> Patched:
>>> ToSet.toSetParallel1  true  avgt   50 62,121 ±   1,207  
>>> us/op
>>> ToSet.toSetParallel   10  true  avgt   50556,968 ±  37,404  
>>> us/op
>>> ToSet.toSetParallel  100  true  avgt   50   6572,372 ± 183,466  
>>> us/op
>>> 
>>> When data is evenly distributed, no statistically significant changes
>>> observed:
>>> 
>>> Original:
>>> ToSet.toSetParallel1 false  avgt   50177,137 ±5,941  
>>> us/op
>>> ToSet.toSetParallel   10 false  avgt   50   2056,332 ±   87,433  
>>> us/op
>>> ToSet.toSetParallel  100 false  avgt   50  51864,198 ±  560,883  
>>> us/op
>>> Patched:
>>> ToSet.toSetParallel1 false  avgt   50172,606 ±   7,321  
>>> us/op
>>> ToSet.toSetParallel   10 false  avgt   50   1922,478 ±  79,593  
>>> us/op
>>> ToSet.toSetParallel  100 false  avgt   50  52648,100 ± 621,526  
>>> us/op
>>> 
>>> What do you think? Should I open an issue?
>>> 
>>> With best regards,
>>> Tagir Valeev.
>>> 
> 

Re: RFR 9 7180225 : SecurityExceptions not defined in some class loader methods

[Brent Christian]

On 8/16/16 8:33 PM, Mandy Chung wrote:

On Aug 16, 2016, at 1:54 PM, Brent Christian  wrote:

Bug:  https://bugs.openjdk.java.net/browse/JDK-7180225
Webrev:   http://cr.openjdk.java.net/~bchristi/7180225/webrev.01/webrev/
Specdiff: 
http://cr.openjdk.java.net/~bchristi/7180225/webrev.01/specdiff/package-summary.html


Looks fine.  A typo in Thread.java “(“ should be “{"

1520  * @return  the context (@code ClassLoader} for this thread, or {@code 
null}


Good catch - I'll fix it before pushing.

Thank you for the reviews, Mandy and Sean.

-Brent

Re: RFR (JAXP): 8146961: Fix PermGen memory leaks caused by static final Exceptions

[Joe Wang]

Looks good, Aleksej. Thanks for coming up with a well-thought solution 
to get rid of the static RE field.


Best,
Joe

On 8/17/16, 7:04 AM, Aleks Efimov wrote:

Hi Joe, Aleksey,
Thank you for reviewing the initial fix.

I followed the Joe's suggestion (thanks for that) and removed static 
'abort' field completely, the functionality was replaced by throwing 
the exception of newly added type - AbortException. The new webrev 
with removed 'abort' can be found here:

http://cr.openjdk.java.net/~aefimov/8146961/9/01

The Tomcat reproducer attached to the bug report fails without the fix 
and passes with the new version of the fix.

The JPRT and JCK testing again shows no related jaxp tests failures.

Best Regards,
Aleksej


On 15/08/16 21:09, Joe Wang wrote:

Hi Aleksej,

I suggest we get rid of the static abort. If RuntimeException 
happens, check where it happens. The first use case looks suspicious 
as it just returns if it's an instance of RE. For the 2nd case in DOM 
error report, let's throw a RuntimeException with the specified error 
message if error happens, and there's no handler or the handler 
failed to handle it. (would be better than an empty RE)


Best,
Joe

On 8/15/16, 10:38 AM, Aleks Efimov wrote:

Hi,

Please, help to review the fix for memory leak [1] in 
com.sun.org.apache.xerces.internal.dom.DOMNormalizer that is caused 
by usage of static final exceptions.
This problem was already fixed in Apache Xerces project [2] and I 
would like to backport it to JDK9 Xerces implementation.

Webrev with the changes:
http://cr.openjdk.java.net/~aefimov/8146961/9/00

The Tomcat reproducer attached to the bug report fails without the 
fix and passes with the fix.
The JPRT and JCK testing shows no jaxp tests failures with the 
proposed fix.


With Best Regards,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8146961
[2] https://issues.apache.org/jira/browse/XERCESJ-1667

Re: RFR 9 7180225 : SecurityExceptions not defined in some class loader methods

[Sean Mullan]

Looks fine to me.

--Sean

On 08/16/2016 02:54 PM, Brent Christian wrote:

Hi,

Please review this change which does some cleanup around documenting
conditions for throwing SecurityExceptions.

It adds a missing @throws tag to Class.forName(String, boolean,
ClassLoader), and consolidates specifics in the @throws tags of
ClassLoader & Thread.

Bug:  https://bugs.openjdk.java.net/browse/JDK-7180225
Webrev:   http://cr.openjdk.java.net/~bchristi/7180225/webrev.01/webrev/
Specdiff:
http://cr.openjdk.java.net/~bchristi/7180225/webrev.01/specdiff/package-summary.html


Thanks!

-Brent

Re: RFR (JAXP): 8146961: Fix PermGen memory leaks caused by static final Exceptions

[Aleks Efimov]

Hi Joe, Aleksey,
Thank you for reviewing the initial fix.

I followed the Joe's suggestion (thanks for that) and removed static 
'abort' field completely, the functionality was replaced by throwing the 
exception of newly added type - AbortException. The new webrev with 
removed 'abort' can be found here:

http://cr.openjdk.java.net/~aefimov/8146961/9/01

The Tomcat reproducer attached to the bug report fails without the fix 
and passes with the new version of the fix.

The JPRT and JCK testing again shows no related jaxp tests failures.

Best Regards,
Aleksej


On 15/08/16 21:09, Joe Wang wrote:

Hi Aleksej,

I suggest we get rid of the static abort. If RuntimeException happens, 
check where it happens. The first use case looks suspicious as it just 
returns if it's an instance of RE. For the 2nd case in DOM error 
report, let's throw a RuntimeException with the specified error 
message if error happens, and there's no handler or the handler failed 
to handle it. (would be better than an empty RE)


Best,
Joe

On 8/15/16, 10:38 AM, Aleks Efimov wrote:

Hi,

Please, help to review the fix for memory leak [1] in 
com.sun.org.apache.xerces.internal.dom.DOMNormalizer that is caused 
by usage of static final exceptions.
This problem was already fixed in Apache Xerces project [2] and I 
would like to backport it to JDK9 Xerces implementation.

Webrev with the changes:
http://cr.openjdk.java.net/~aefimov/8146961/9/00

The Tomcat reproducer attached to the bug report fails without the 
fix and passes with the fix.
The JPRT and JCK testing shows no jaxp tests failures with the 
proposed fix.


With Best Regards,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8146961
[2] https://issues.apache.org/jira/browse/XERCESJ-1667

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Peter Levart]

Hi Kim,


On 08/15/2016 05:15 AM, Kim Barrett wrote:

>I have a feeling that these pauses are now unnecessary. Will try to check with 
some experiments…

I found that the DirectBufferAllocTest will sometimes fail if the pauses are 
taken out.
I think what’s going on is that the multiple threads are competing for 
resources, and
some threads in that test lose out if all of them are waiting and wake up at 
the same
time.  The exponentially increasing back-off scatters the threads enough for 
that to
become very unlikely, though with sufficiently bad luck… But I think the current
implementation could also fail that test with similarly bad luck.  It just 
requires*very*
bad luck, so we’re not seeing it as a problem.  And that test is a pretty 
extreme stress
test.



I get the same results when experimenting with this. Another option 
would be to enclose the whole logic of (retrying reservation while 
waiting for Reference processing progress followed by System.gc() and 
another round of reservation retries while waiting for Reference 
processing) into a synchronized block so that multiple reservation 
threads could not barge for reservations. This approach works and might 
even be triggering less System.gc() rounds, but it is not much better 
than current approach.


Regards, Peter

Exceptional behavior of parallel stream

[Tagir F. Valeev]

Hello!

I found no information in Stream API documentation on how it behaves
in case if exception occurs during the stream processing. While it's
quite evident for sequential stream (stream processing is terminated
and exception is propagated to the caller as is), the behavior for
parallel streams differs from one might expect. Consider the following
test:

String[] data = IntStream.range(0, 100).mapToObj(String::valueOf)
 .toArray(String[]::new);
data[20] = "oops";
try {
int sum = Arrays.stream(data).parallel().mapToInt(Integer::valueOf)
.peek(System.out::println).sum();
System.out.println("Sum is "+sum);
} catch (NumberFormatException e) {
System.out.println("Non-number appeared!");
}

This parses integers stored in string array and sums them outputting
every number to stdout once it processed. As data set contains
non-number, the stream obviously fails with NumberFormatException. The
typical output looks like this:

62
63
12
31
87
...
28
92
29
8
Non-number appeared!
9
30

So as you can see, the stream is not actually terminated when
exception is thrown and caught: even after that some parallel tasks
continue running, and you see more numbers printed after catch block
is executed.

I consider such behavior as confusing and unexpected. Given the fact
that stream may produce side-effects (e.g. if terminal op is forEach)
this might lead to unforeseen consequences in user programs as
left-over parallel stream tasks may continue mutate shared state after
main stream thread exceptionally returns the control to the caller.

So I suggest that such behavior should be either fixed or explicitly
documented. What do you think?

With best regards,
Tagir Valeev.

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

[Daniel Fuchs]

Hi Peter,

On 17/08/16 12:20, Peter Levart wrote:

Hi Daniel,

Thinking of this patch from the compatibility standpoint, aren't you
afraid that someone might be using the following idiom:

public class MyLevel extends java.util.logging.Level {
static {
new MyLevel("LEVEL1", 1);
new MyLevel("LEVEL2", 2);
...
}

public static void init() {}

private MyLevel(String name, int value) {
super(name, value);
}
...
}

...and then use those levels in code like:

MyLevel.init();
...
Level level = Level.parse("LEVEL1") ...


That's a possibility, and maybe that would deserve to be noted as
a side effect of the fix in the release notes.

I do not believe this is something we would want to actively support
though: the usual idiom for custom level would rather be to define
them as public final static constant, and use a direct reference
in the code.

Something like:

public class MyLevel extends Level {

public static final MyLevel LEVEL1 = new MyLevel("LEVEL1", 1);
...
}

   Level level = MyLevel.LEVEL1;

or even more simpler:

   public static class Levels {

  public static Level LEVEL1 = new Level("LEVEL1", 1) {};
  ...
   }


So maybe this is a risk we should take. Mandy, what do you think?

The other alternative is not to fix the bug as it's been here
for several releases ;-)

-- daniel




With the proposed patch, this will not work any more as custom Level
subclass instances will possibly be GCed before being "parsed" and
retained.

To prevent that from happening and still allow custom Level subclasses
and their class loaders to be garbage-collectable, I recommend that you
"pin" each constructed subclass instance to its ClassLoader with a
strong reference. You could use a ClassLoaderValue for this purpose,
like in the following addition to your patch (see KnownLevel.add):

http://cr.openjdk.java.net/~plevart/jdk9-dev/Level.known/webrev.01/

Regards, Peter


On 08/16/2016 12:42 PM, Daniel Fuchs wrote:

Hi Mandy,

I added an additional selector parameter to the find methods.
This made it possible to return Optional instead of
KnownLevel - and it does simply the parse() method.

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

best regards,

-- daniel

On 11/08/16 20:12, Mandy Chung wrote:



On Aug 11, 2016, at 2:29 AM, Daniel Fuchs 
wrote:

On 10/08/16 17:21, Mandy Chung wrote:

On Jul 29, 2016, at 4:54 AM, Daniel Fuchs
 wrote:


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

This looks pretty good.

Since KnownLevel is now a Reference, I suggest to change
KnownLevel::findByName, findByValue and findByLocalizedLevelName to
return Optional instead such that the parse method
implementaiton could be simplified.


We need to return KnownLevel because sometimes we need the
level object and sometimes the mirror.


So either findByName(String name, boolean mirror) or two methods:
findLevelByName and findMirroredLevelByName??

Or seriously consider to remove KnownLevel class by introducing a new
Level subclass with final Level.getName, Level.getLocalizedName,
Level.getResourceBundleName methods??

Mandy

Re: RFE to re-purpose option --version:

[Alan Bateman]

On 10/08/2016 23:46, Martin Buchholz wrote:

On Wed, Aug 10, 2016 at 11:52 AM, Paul Benedict 
wrote:


Martin, what do you have in mind when saying it's "broken"? Functionally,
everything is fine -- I can delete "release" locally without issue (so it
appears) -- so I imagine there's humor here I am missing? :-)


The release file is a user interface.  Removing it is just like removing
any other part of the product (e.g. bin/javap) that users may have built a
dependency on.  It's broken, just not totally broken!
Right, the `release` is a JDK-specific and supported interface. It has 
been there for several releases to allow tools inspect the image without 
needing to invoke `java -version`. Removing the release file seems 
misguided. Also something like `java --version:9.1` isn't going to work 
when the run-time image is for another OS/architecture, another use-case 
for the `release` file btw.


-Alan

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

[Peter Levart]

On 08/17/2016 01:20 PM, Peter Levart wrote:
You could use a ClassLoaderValue for this purpose, like in the 
following addition to your patch (see KnownLevel.add):


http://cr.openjdk.java.net/~plevart/jdk9-dev/Level.known/webrev.01/


Note that the above also contains the functional-style findLevel and 
parse implementations which you would probably want to leave out...


Regards, Peter

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

[Peter Levart]

On 08/17/2016 01:16 PM, Daniel Fuchs wrote:

Now that you have various findByXXX methods return Optional, you
could make methods that use them more Optional-API-powered. For 
example,

findLevel could be written as a single expression:


Right. But do you mind if I leave that for another day?
I'm not too keen on multi-line code statements embedded
in lambdas - but I feel like we did enough restructuring
for this fix now ;-) 


It's just that the functional style is actually more readable in this 
case. But never mind. It's just style.


Regards, Peter

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

[Peter Levart]

Hi Daniel,

Thinking of this patch from the compatibility standpoint, aren't you 
afraid that someone might be using the following idiom:


public class MyLevel extends java.util.logging.Level {
static {
new MyLevel("LEVEL1", 1);
new MyLevel("LEVEL2", 2);
...
}

public static void init() {}

private MyLevel(String name, int value) {
super(name, value);
}
...
}

...and then use those levels in code like:

MyLevel.init();
...
Level level = Level.parse("LEVEL1") ...


With the proposed patch, this will not work any more as custom Level 
subclass instances will possibly be GCed before being "parsed" and retained.


To prevent that from happening and still allow custom Level subclasses 
and their class loaders to be garbage-collectable, I recommend that you 
"pin" each constructed subclass instance to its ClassLoader with a 
strong reference. You could use a ClassLoaderValue for this purpose, 
like in the following addition to your patch (see KnownLevel.add):


http://cr.openjdk.java.net/~plevart/jdk9-dev/Level.known/webrev.01/

Regards, Peter


On 08/16/2016 12:42 PM, Daniel Fuchs wrote:

Hi Mandy,

I added an additional selector parameter to the find methods.
This made it possible to return Optional instead of
KnownLevel - and it does simply the parse() method.

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

best regards,

-- daniel

On 11/08/16 20:12, Mandy Chung wrote:


On Aug 11, 2016, at 2:29 AM, Daniel Fuchs  
wrote:


On 10/08/16 17:21, Mandy Chung wrote:
On Jul 29, 2016, at 4:54 AM, Daniel Fuchs 
 wrote:


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

This looks pretty good.

Since KnownLevel is now a Reference, I suggest to change 
KnownLevel::findByName, findByValue and findByLocalizedLevelName to 
return Optional instead such that the parse method 
implementaiton could be simplified.


We need to return KnownLevel because sometimes we need the
level object and sometimes the mirror.


So either findByName(String name, boolean mirror) or two methods: 
findLevelByName and findMirroredLevelByName??


Or seriously consider to remove KnownLevel class by introducing a new 
Level subclass with final Level.getName, Level.getLocalizedName, 
Level.getResourceBundleName methods??


Mandy

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

[Daniel Fuchs]

Hi Peter,


Now that you have various findByXXX methods return Optional, you
could make methods that use them more Optional-API-powered. For example,
findLevel could be written as a single expression:


Right. But do you mind if I leave that for another day?
I'm not too keen on multi-line code statements embedded
in lambdas - but I feel like we did enough restructuring
for this fix now ;-)

From  your other mail:

Oh, yes. I missed Chris' comment. Perhaps just add a comment about that for 
posterity?


I added the following comment to stress out that
a reachability fence is not needed (at the two places
where it occurred):

 479 // add new Level.
 480 Level levelObject = new Level(name, x);
 481 // There's no need to use a reachability fence here because
 482 // KnownLevel keeps a strong reference on the level when
 483 // level.getClass() == Level.class.
 484 return KnownLevel.findByValue(x, KnownLevel::referent).get();

For the record here are the changes - including Mandy's suggestions.
If there's no objection I will push them after testing has passed,
since Mandy said she didn't require a new webrev.

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

best regards,

-- daniel

On 16/08/16 14:54, Peter Levart wrote:

Hi Daniel,

Now that you have various findByXXX methods return Optional, you
could make methods that use them more Optional-API-powered. For example,
findLevel could be written as a single expression:

static Level findLevel(String name) {
return KnownLevel
.findByName(Objects.requireNonNull(name), KnownLevel::mirrored)
.or(() -> {
// Now, check if the given name is an integer.  If so,
// first look for a Level with the given value and then
// if necessary create one.
try {
int x = Integer.parseInt(name);
return KnownLevel
.findByValue(x, KnownLevel::mirrored)
.or(() -> {
// add new Level
new Level(name, x);
return KnownLevel.findByValue(x,
KnownLevel::mirrored);
});
} catch (NumberFormatException ex) {
// Not an integer.
// Drop through.
return Optional.empty();
}
})
.or(() -> KnownLevel.findByLocalizedLevelName(name,
KnownLevel::mirrored))
.orElse(null);
}


Same with parse:

public static synchronized Level parse(String name) throws
IllegalArgumentException {
return KnownLevel
// Look for a known Level with the given non-localized name.
.findByName(Objects.requireNonNull(name), KnownLevel::referent)
.or(() -> {
// Now, check if the given name is an integer.  If so,
// first look for a Level with the given value and then
// if necessary create one.
try {
int x = Integer.parseInt(name);
return KnownLevel
.findByValue(x, KnownLevel::referent)
.or(() -> {
// add new Level.
new Level(name, x);
return KnownLevel.findByValue(x,
KnownLevel::referent);
});
} catch (NumberFormatException ex) {
// Not an integer.
// Drop through.
return Optional.empty();
}
})
// Finally, look for a known level with the given localized
name,
// in the current default locale.
// This is relatively expensive, but not excessively so.
.or(() -> KnownLevel.findByLocalizedLevelName(name,
KnownLevel::referent))
// OK, we've tried everything and failed
.orElseThrow(() -> new IllegalArgumentException("Bad level
\"" + name + "\""));
}


Regards, Peter


On 08/16/2016 12:42 PM, Daniel Fuchs wrote:

Hi Mandy,

I added an additional selector parameter to the find methods.
This made it possible to return Optional instead of
KnownLevel - and it does simply the parse() method.

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

best regards,

-- daniel

On 11/08/16 20:12, Mandy Chung wrote:



On Aug 11, 2016, at 2:29 AM, Daniel Fuchs 
wrote:

On 10/08/16 17:21, Mandy Chung wrote:

On Jul 29, 2016, at 4:54 AM, Daniel Fuchs
 wrote:


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

This looks pretty good.

Since KnownLevel is now a Reference, I suggest to change
KnownLevel::findByName, findByValue and findByLocalizedLevelName to
return Optional instead such that the parse method
implementaiton could be 

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

[Daniel Fuchs]

Hi Peter,

Rereading the comment and looking at the code I see
that the comment is actually right. There are two methods
with similar names:

Level.getLocalizedName() which can be overridden by subclasses
and Level.getLocalizedLevelName() which cannot.

By default Level.getLocalizedName() calls Level.getLocalizedLevelName(),
and  findByLocalizedLevelName() calls Level.getLocalizedLevelName() too,
not the overridable Level.getLocalizedName().

best regards,

-- daniel

On 16/08/16 13:59, Peter Levart wrote:

Hi Daniel,

Another place that is not clear to me is the following (in old code):

 585 // Returns a KnownLevel with the given localized name matching
 586 // by calling the Level.getLocalizedLevelName() method (i.e. found
 587 // from the resourceBundle associated with the Level object).
 588 // This method does not call Level.getLocalizedName() that may
 589 // be overridden in a subclass implementation
590 static synchronized KnownLevel findByLocalizedLevelName(String name) {
591 for (List levels : nameToLevels.values()) {
592 for (KnownLevel l : levels) {
593 String lname = l.levelObject.getLocalizedLevelName();
594 if (name.equals(lname)) {
595 return l;
596 }
597 }
598 }
599 return null;
 600 }

In spite of claiming that "this method doesn't call
Level.getLocalizedName() that may be overridden in subclass", it does
just that. it calls the getLocalizedLevelName on the
KnownLevel.levelObject, which is a user-constructed object. You changed
the code into:

 627 static synchronized Optional
findByLocalizedLevelName(String name,
 628 Function selector) {
 629 purge();
 630 return nameToLevels.values()
 631 .stream()
 632 .flatMap(List::stream)
 633 .flatMap(selector)
 634 .filter(lo ->
name.equals(lo.getLocalizedLevelName()))
 635 .findFirst();
 636 }

Which calls getLocalizedName() on the Level object selected by the given
selector. In line 385 you pass in the selector to select the
mirroredLevel, but in line 487, you pass in the selector to select the
referent.

So which one is the right one? If you want to obey the comment it should
always be the mirroredLevel which is checked against localized name, right?

Regards, Peter

Re: RFR: JDK-8161230 - Create java.util.stream.Stream from Iterator / Enumeration

[Patrick Reinhart]

On 17.08.2016 01:50, Paul Sandoz wrote:
> On 8 Aug 2016, at 12:14, Patrick Reinhart  wrote:
>> Am 08.08.2016 um 18:55 schrieb Alan Bateman :
>>> On 08/08/2016 17:29, Patrick Reinhart wrote:
>>>
 :
 I tried to integrate your suggested changes here:
 http://cr.openjdk.java.net/~reinhapa/reviews/8161230/ClassLoader_StreamMethods.01

>>> I should have been clearer. A lazy implementation of 
>>> resources/systemResources methods won't throw any exceptions, instead any 
>>> I/O exceptions will be wrapped with an UncheckedIOException and then thrown 
>>> from the method that caused the access to take place. There are several 
>>> examples of this already. For the javadoc then this will be described in 
>>> the method description rather than a @throws.
>>>
>>> -Alan
>> I hope that this version is more likely that what you meant…
>>
>> http://cr.openjdk.java.net/~reinhapa/reviews/8161230/ClassLoader_StreamMethods.02
>>  
>> 
>>
> Perhaps consider:
>
>   The loading of resources will occur when the returned stream is evaluated. 
> If the loading of resources
>   results in an {@code IOException} then the I/O exception is wrapped in an 
> {@link UncheckedIOException}
>   that is then thrown.
>
> Instead of … use {@code … }
>
> Paul.

Hi Paul,

Thanks for the input. I integrated that into the version here:

http://cr.openjdk.java.net/~reinhapa/reviews/8161230/ClassLoader_StreamMethods.03



Patrick

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

[Daniel Fuchs]

Hi Mandy,

On 17/08/16 05:05, Mandy Chung wrote:

On Aug 16, 2016, at 5:42 AM, Daniel Fuchs  wrote:
>
> Hi Mandy,
>
> I added an additional selector parameter to the find methods.
> This made it possible to return Optional instead of
> KnownLevel - and it does simply the parse() method.
>
> http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.02
>

This looks much better.  I like the findByName method taking a function to select what to 
be returned.  But the selector looks strange to map from a KnownLevel to a stream of Level. 
 I think it should be Function and the callsite 
can convert to Stream using Optional::stream.


OK - I'll do that.


No need for me to see an updated webrev.

thanks
Mandy