Re: RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command

[serguei.spit...@oracle.com]

On 4/15/16 11:59, Dmitry Samersoff wrote:

Severin,

Looks good for me.

But I'm a little afraid of the fact that now we are holding
eventHandler_lock while doing invoke*.


It seems, I have this concern too.
Please, let me take a look closer at this part if it is done in a right way.




So please hold on with backports until the fix bakes in jdk9 for some time.

+1

Thanks,
Serguei



-Dmitry

On 2016-04-15 19:53, Severin Gehwolf wrote:

Hi,

Here is a patch which is a redo of the fix for JDK-4858370 which got
backed out due to it causing test regressions. Specifically problems
were reported for com/sun/jdi/InvokeTest.java
and com/sun/jdi/InterfaceMethodsTest.java with the fix for JDK-4858370
applied.

Those test regressions were caused because:
a.) The fix for JDK-4858370 deleted refs from the request object
 while *not* holding the invoker lock.
b.) Invokes done via invoker_doInvoke() save global references, but
 don't hold the invoker lock either.

This new fix grabs relevant locks for both cases above.

Testing done:
  - com/sun/jdi test set. No regressions + added regression test.
  - com/sun/jdi/InterfaceMethodsTest.java did not fail in 1300
invocations. Same for com/sun/jdi/InvokeTest.java.
  - I haven't seen any crashes in new OomDebugTest.java either.

Bug: https://bugs.openjdk.java.net/browse/JDK-8153711
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/

Once reviewed, I'd need somebody to sponsor this patch.

Thanks,
Severin

Re: RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command

[Dmitry Samersoff]

Severin,

Looks good for me.

But I'm a little afraid of the fact that now we are holding
eventHandler_lock while doing invoke*.

So please hold on with backports until the fix bakes in jdk9 for some time.

-Dmitry

On 2016-04-15 19:53, Severin Gehwolf wrote:
> Hi,
> 
> Here is a patch which is a redo of the fix for JDK-4858370 which got
> backed out due to it causing test regressions. Specifically problems
> were reported for com/sun/jdi/InvokeTest.java
> and com/sun/jdi/InterfaceMethodsTest.java with the fix for JDK-4858370
> applied.
> 
> Those test regressions were caused because:
> a.) The fix for JDK-4858370 deleted refs from the request object
> while *not* holding the invoker lock.
> b.) Invokes done via invoker_doInvoke() save global references, but
> don't hold the invoker lock either.
> 
> This new fix grabs relevant locks for both cases above.
> 
> Testing done:
>  - com/sun/jdi test set. No regressions + added regression test.
>  - com/sun/jdi/InterfaceMethodsTest.java did not fail in 1300
>invocations. Same for com/sun/jdi/InvokeTest.java.
>  - I haven't seen any crashes in new OomDebugTest.java either.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8153711
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/
> 
> Once reviewed, I'd need somebody to sponsor this patch.
> 
> Thanks,
> Severin
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: 8149790: NegativeArraySizeException with hprof

[Erik Gahlin]

Looks good, not a Reviewer.

Do you really need curly braces in the switch clauses?

Erik

On 2016-04-15 16:40, Andreas Eriksson wrote:

Hi,

Please review this test fix for 8149790: NegativeArraySizeException 
with hprof


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

Changes are to the hprof verifier, which now will pass heap dump 
content around as JavaThing arrays instead of byte arrays, since the 
latter cannot be guaranteed to be able to hold all the elements of 
large arrays.


There is still a problem where the test will timeout on machines with 
lots of memory (seen on machines with 200+GB of memory) because the 
verification takes a long time. I'll file a new bug for that problem.


Regards,
Andreas

Re: RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command

[serguei.spit...@oracle.com]

Hi Severin,

The fix looks good to me.
I can sponsor the fix once it is reviewed.

Not sure, if I need to run the previously failed tests in the big loops 
or rely on your testing.

Included Dan who did a back out into the cc-list.

Thanks a lot for taking care to redo the fix!

Thanks,
Serguei


On 4/15/16 09:53, Severin Gehwolf wrote:

Hi,

Here is a patch which is a redo of the fix for JDK-4858370 which got
backed out due to it causing test regressions. Specifically problems
were reported for com/sun/jdi/InvokeTest.java
and com/sun/jdi/InterfaceMethodsTest.java with the fix for JDK-4858370
applied.

Those test regressions were caused because:
a.) The fix for JDK-4858370 deleted refs from the request object
 while *not* holding the invoker lock.
b.) Invokes done via invoker_doInvoke() save global references, but
 don't hold the invoker lock either.

This new fix grabs relevant locks for both cases above.

Testing done:
  - com/sun/jdi test set. No regressions + added regression test.
  - com/sun/jdi/InterfaceMethodsTest.java did not fail in 1300
invocations. Same for com/sun/jdi/InvokeTest.java.
  - I haven't seen any crashes in new OomDebugTest.java either.

Bug: https://bugs.openjdk.java.net/browse/JDK-8153711
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/

Once reviewed, I'd need somebody to sponsor this patch.

Thanks,
Severin

Re: RFR: 8153749 - New capability can_generate_early_class_hook_events

[serguei.spit...@oracle.com]

On 4/15/16 04:00, Alan Bateman wrote:



On 14/04/2016 21:30, serguei.spit...@oracle.com wrote:

Alan,

This is for sanity check:

The updated hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8153749-Jigsaw-newcap.hs2/

Please, note that the *src/share/vm/prims/jvmtiEnvBase.hpp* was 
corrected too.


In can_generate_early_class_hook_events then "can be posted" or "may 
be posted" might be better than "could be posted". Also the end tag 
'>' at L10002 should be probably be on the proceeding line.


Fixed, thanks.



Otherwise looks okay to me.


Thanks, Alan!
Serguei



-Alan

RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command

[Severin Gehwolf]

Hi,

Here is a patch which is a redo of the fix for JDK-4858370 which got
backed out due to it causing test regressions. Specifically problems
were reported for com/sun/jdi/InvokeTest.java
and com/sun/jdi/InterfaceMethodsTest.java with the fix for JDK-4858370
applied.

Those test regressions were caused because:
a.) The fix for JDK-4858370 deleted refs from the request object
    while *not* holding the invoker lock.
b.) Invokes done via invoker_doInvoke() save global references, but
    don't hold the invoker lock either.

This new fix grabs relevant locks for both cases above.

Testing done:
 - com/sun/jdi test set. No regressions + added regression test.
 - com/sun/jdi/InterfaceMethodsTest.java did not fail in 1300
   invocations. Same for com/sun/jdi/InvokeTest.java.
 - I haven't seen any crashes in new OomDebugTest.java either.

Bug: https://bugs.openjdk.java.net/browse/JDK-8153711
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/

Once reviewed, I'd need somebody to sponsor this patch.

Thanks,
Severin

RFR: 8149790: NegativeArraySizeException with hprof

[Andreas Eriksson]

Hi,

Please review this test fix for 8149790: NegativeArraySizeException with 
hprof


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

Changes are to the hprof verifier, which now will pass heap dump content 
around as JavaThing arrays instead of byte arrays, since the latter 
cannot be guaranteed to be able to hold all the elements of large arrays.


There is still a problem where the test will timeout on machines with 
lots of memory (seen on machines with 200+GB of memory) because the 
verification takes a long time. I'll file a new bug for that problem.


Regards,
Andreas

Re: PerfData counter: sun.gc.policy.generations in JDK 8

[Stefan Karlsson]

Hi Ramki and Jon,

What's the status of this review thread? The bug is still open and 
targeted for JDK 9.


Thanks,
StefanK

On 2015-06-03 08:15, Srinivas Ramakrishna wrote:
Thanks Jon for the review and the pointer to the test. I'll get back 
to you later this week with a suitable test.


-- Ramki

ysr1729

On Jun 2, 2015, at 14:16, Jon Masamitsu > wrote:



Ramki,

Changes look good.

I'm guessing you tested by generating the
perfdata by hand and verifying the contents
of the perfdata.  Do you think a test can
be written to verify  the change?  If you look at

test/gc/metaspace/TestMetaspacePerfCounters.java

in your repository I think that is an example that
can be followed.

It's a jtreg test.

http://openjdk.java.net/jtreg/

Jon

On 06/01/2015 11:39 AM, Srinivas Ramakrishna wrote:
Thanks for the review of the patch for 8-dev (from the ticket), 
Staffan.


Sorry for the delay in getting the official webrev out -- it took me 
a while to first get set up with an hs9 repo (thanks Jon!) and then 
get my openjdk credentials updated (thanks Mark!).


Here's the webrev against hs9 for official review:-

http://cr.openjdk.java.net/~ysr/JDK-8080345/webrev.00/ 



I built and tested the change (on both 8-dev whose patch was 
attached with the original bug, as well as this with hs9) and 
verified that the counter value for generations, in the perfdata 
file, was now 2 instead of the previous 3.


thanks!
-- ramki


On Mon, May 18, 2015 at 1:22 AM, Staffan Larsen 
> wrote:


Looks like a good patch to me.

/Staffan


On 14 maj 2015, at 18:12, Srinivas Ramakrishna
> wrote:

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



On Wed, May 13, 2015 at 1:08 PM, Srinivas Ramakrishna
> wrote:


With perm gen going away (and being replaced by metaspace)
in JDK 8, it makes sense that the counter
sun.gc.policy.generations should be "2", rather than "3".
However, in JDK 8 that counter still says 3.
As I understand, the intention was that this counter would
allow you to (for example) know the range of
the sun.gc.generation.$num.* counters describing each of
$num < sun.gc.policy.generations in the heap.
Recall that the erstwhile perm gen in JDK 7 used to be
synonymous with sun.gc.generation.2, but the
JDK 8 avatars are now sun.gc.metaspace and
sun.gc.compressedclassspace.

The fix is simple, and I can submit a patch. Is there an
existing bug for this?

thanks!
-- ramki

Re: [8u] RFR: 8153641: assert(thread_state == _thread_in_native) failed: Assumed thread_in_native while heap dump

[Andreas Eriksson]

Thanks Staffan!

- Andreas

On 2016-04-15 13:00, Staffan Larsen wrote:

Looks good!

Thanks,
/Staffan

On 12 apr. 2016, at 18:05, Andreas Eriksson 
> wrote:


Hi,

Please review this fix for 8153641: assert(thread_state == 
_thread_in_native) failed: Assumed thread_in_native while heap dump


Bug: https://bugs.openjdk.java.net/browse/JDK-8153641
Webrev: http://cr.openjdk.java.net/~aeriksso/8153641/webrev.00/

The crash is because changes I did in JDK-8129419 
 do not work with 
solaris logic used for UseVMInterruptibleIO in some cases.
This change reverts the changes I did to the solaris version of 
os::write, and changes heapDumper.cpp to use ::write and ::close 
directly, as it did before.


Thanks,
Andreas

Re: RFR: 8153749 - New capability can_generate_early_class_hook_events

[Alan Bateman]

On 14/04/2016 21:30, serguei.spit...@oracle.com wrote:

Alan,

This is for sanity check:

The updated hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8153749-Jigsaw-newcap.hs2/

Please, note that the *src/share/vm/prims/jvmtiEnvBase.hpp* was 
corrected too.


In can_generate_early_class_hook_events then "can be posted" or "may be 
posted" might be better than "could be posted". Also the end tag '>' at 
L10002 should be probably be on the proceeding line.


Otherwise looks okay to me.

-Alan

Re: [8u] RFR: 8153641: assert(thread_state == _thread_in_native) failed: Assumed thread_in_native while heap dump

[Staffan Larsen]

Looks good!

Thanks,
/Staffan

> On 12 apr. 2016, at 18:05, Andreas Eriksson  
> wrote:
> 
> Hi,
> 
> Please review this fix for 8153641: assert(thread_state == _thread_in_native) 
> failed: Assumed thread_in_native while heap dump
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8153641 
> 
> Webrev: http://cr.openjdk.java.net/~aeriksso/8153641/webrev.00/ 
> 
> 
> The crash is because changes I did in JDK-8129419 
>  do not work with solaris 
> logic used for UseVMInterruptibleIO in some cases.
> This change reverts the changes I did to the solaris version of os::write, 
> and changes heapDumper.cpp to use ::write and ::close directly, as it did 
> before.
> 
> Thanks,
> Andreas