date:20170810

RE: [10] RFR for JDK-8169961: Memory leak after debugging session

2017-08-10 Thread Shafi Ahmad

Thank you Daniel and Serguei for reviewing it.

Please find updated webrev - I added volatile attribute to variable 
'shouldListen'.

http://cr.openjdk.java.net/~shshahma/8169961/webrev.02/

Regards,
Shafi

> -Original Message-
> From: Serguei Spitsyn
> Sent: Thursday, August 10, 2017 12:27 AM
> To: Daniel Daugherty ; Shafi Ahmad
> ; serviceability-dev@openjdk.java.net
> Cc: Roger Calnan 
> Subject: Re: [10] RFR for JDK-8169961: Memory leak after debugging session
> 
> On 8/9/17 08:27, Daniel D. Daugherty wrote:
> > On 8/9/17 3:05 AM, Shafi Ahmad wrote:
> >> May I get it reviewed by  serviceability team.
> >
> > I don't count as being on the Serviceability team anymore, but
> 
> Common, Dan, you are still counted! :)
> 
> > I've pinged Serguei Spitsyn who is back from his vacation...
> >
> > More below...
> 
> More below.
> 
> >
> >
> >>
> >> Regards,
> >> Shafi
> >>
> >>
> >>
> >>> -Original Message-
> >>> From: Shafi Ahmad
> >>> Sent: Wednesday, July 26, 2017 8:26 AM
> >>> To: serviceability-dev@openjdk.java.net
> >>> Cc: Roger Calnan 
> >>> Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging
> >>> session
> >>>
> >>> May I get it reviewed by someone from serviceability group.
> >>>
> >>> Webrev link:
> http://cr.openjdk.java.net/~shshahma/8169961/webrev.01/
> >>> This review thread:
> >>> http://mail.openjdk.java.net/pipermail/serviceability-
> >>> dev/2017-July/021538.html
> >>>
> >>> Regards,
> >>> Shafi
> >>>
>  -Original Message-
>  From: Shafi Ahmad
>  Sent: Monday, July 24, 2017 12:52 PM
>  To: serviceability-dev@openjdk.java.net
>  Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging
>  session
> 
>  Hi,
> 
> > -Original Message-
> > From: Langer, Christoph [mailto:christoph.lan...@sap.com]
> > Sent: Monday, July 17, 2017 9:01 PM
> > To: Poonam Parhar 
> > Cc: Shafi Ahmad ; serviceability-
> > d...@openjdk.java.net
> > Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging
> > session
> >
> > Hi Poonam,
> >
> >
> >> Line 182: Here, eventController.release() is called after the 'vm'
> >> is
>  disposed.
> >> And eventController.release() causes the following statement to
> >> be executed on the eventcontroller thread after the 'vm' is
> disposed:
> >>
> >> JDWP.VirtualMachine.ReleaseEvents.process(vm);
> >>
> >> Which does not seem to be right. Someone from the Serviceability
> >> group can confirm the correctness of this change.
> > I think this is okay, because with the new change shouldListen()
> > is called right after the thread returns from wait(). And this
> > will lead to the thread immediately exiting.
> > JDWP.VirtualMachine.ReleaseEvents.process(vm);
> > should not be called in this case.
> >
> > I just re-read the webrev and I agree with Christoph that this new check:
> >
> >   L358: if (!shouldListen) {
> >   L359:return;
> >   L360: }
> >
> > will keep the EventController.run() method from trying to use the 'vm'
> > that has been disposed.
> 
> Agreed.
> The only issue is that the 'shouldListen' field has to be volatile now.
> 
> Otherwise, the fix looks good to me.
> 
> Thank you for taking care about this issue!
> 
> Thanks,
> Serguei
> 
> >
> > Dan
> >
> >
> >
> >> Line 330: Instance variable 'VirtualMachineImpl vm' is removed
> >> from the EventController class. It is being used further down in
> >> its
> >> run() method. So I think it cannot be removed.
> > The vm object is used from the outer class TargetVM, as
> > EventController is an inner class of it.
> >
> > So in my view it's all correct but still somebody of the
> > serviceability group might know better...
>  Could someone from serviceability group review this.
> 
>  Regards,
>  Shafi
> 
> > Best regards
> > Christoph
> >
> >
>

Re: [10] RFR for JDK-8169961: Memory leak after debugging session

2017-08-10 Thread serguei.spit...@oracle.com


Hi Shafi,

Looks good.

Thanks,
Serguei


On 8/10/17 00:08, Shafi Ahmad wrote:

Thank you Daniel and Serguei for reviewing it.

Please find updated webrev - I added volatile attribute to variable 
'shouldListen'.

http://cr.openjdk.java.net/~shshahma/8169961/webrev.02/

Regards,
Shafi


-Original Message-
From: Serguei Spitsyn
Sent: Thursday, August 10, 2017 12:27 AM
To: Daniel Daugherty ; Shafi Ahmad
; serviceability-dev@openjdk.java.net
Cc: Roger Calnan 
Subject: Re: [10] RFR for JDK-8169961: Memory leak after debugging session

On 8/9/17 08:27, Daniel D. Daugherty wrote:

On 8/9/17 3:05 AM, Shafi Ahmad wrote:

May I get it reviewed by  serviceability team.

I don't count as being on the Serviceability team anymore, but

Common, Dan, you are still counted! :)


I've pinged Serguei Spitsyn who is back from his vacation...

More below...

More below.




Regards,
Shafi




-Original Message-
From: Shafi Ahmad
Sent: Wednesday, July 26, 2017 8:26 AM
To: serviceability-dev@openjdk.java.net
Cc: Roger Calnan 
Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging
session

May I get it reviewed by someone from serviceability group.

Webrev link:

http://cr.openjdk.java.net/~shshahma/8169961/webrev.01/

This review thread:
http://mail.openjdk.java.net/pipermail/serviceability-
dev/2017-July/021538.html

Regards,
Shafi


-Original Message-
From: Shafi Ahmad
Sent: Monday, July 24, 2017 12:52 PM
To: serviceability-dev@openjdk.java.net
Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging
session

Hi,


-Original Message-
From: Langer, Christoph [mailto:christoph.lan...@sap.com]
Sent: Monday, July 17, 2017 9:01 PM
To: Poonam Parhar 
Cc: Shafi Ahmad ; serviceability-
d...@openjdk.java.net
Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging
session

Hi Poonam,



Line 182: Here, eventController.release() is called after the 'vm'
is

disposed.

And eventController.release() causes the following statement to
be executed on the eventcontroller thread after the 'vm' is

disposed:

JDWP.VirtualMachine.ReleaseEvents.process(vm);

Which does not seem to be right. Someone from the Serviceability
group can confirm the correctness of this change.

I think this is okay, because with the new change shouldListen()
is called right after the thread returns from wait(). And this
will lead to the thread immediately exiting.
JDWP.VirtualMachine.ReleaseEvents.process(vm);
should not be called in this case.

I just re-read the webrev and I agree with Christoph that this new check:

   L358: if (!shouldListen) {
   L359:return;
   L360: }

will keep the EventController.run() method from trying to use the 'vm'
that has been disposed.

Agreed.
The only issue is that the 'shouldListen' field has to be volatile now.

Otherwise, the fix looks good to me.

Thank you for taking care about this issue!

Thanks,
Serguei


Dan



Line 330: Instance variable 'VirtualMachineImpl vm' is removed
from the EventController class. It is being used further down in
its
run() method. So I think it cannot be removed.

The vm object is used from the outer class TargetVM, as
EventController is an inner class of it.

So in my view it's all correct but still somebody of the
serviceability group might know better...

Could someone from serviceability group review this.

Regards,
Shafi


Best regards
Christoph

RE: [10] RFR for JDK-8169961: Memory leak after debugging session

Re: [10] RFR for JDK-8169961: Memory leak after debugging session

2 matches

Site Navigation

Mail list logo

Footer information