Re: [8u] RFR: 8059038: Create new launcher for SA tools

[Andrew Hughes]

On Thu, 13 Dec 2018 at 17:19, Severin Gehwolf  wrote:
>
> On Thu, 2018-12-13 at 15:49 +, Andrew Hughes wrote:
> > Ok, I presume that's not a change you're also going to make in
> > OpenJDK 12?
>
> Yes. I don't intend to change this in JDK 12. JDK 8 and JDK 9+ are not
> compatible in this regard. That's the case before and after this patch.
>
> > I'm fine with them being linked by the same bug ID, as they resolve the same
> > underlying issue, but this really needs to be clear in the summary
> > text, as, to the
> > uninformed, they look like completely different patches.
>
> OK will do. Can I consider this reviewed?
>
> Thanks,
> Severin
>

I'd like to see an updated webrev with a more detailed commit message first.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222

Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss corresponding Release

[David Holmes]

On 19/12/2018 3:19 am, Baesken, Matthias wrote:

Hello, here  is an updated  webrev :


http://cr.openjdk.java.net/~mbaesken/webrevs/8215411.1/

sawindbg.cpp  has been adjusted .
The exception cases mentioned  now also  call   env->ReleaseByteArrayElements  .


Looks good - thanks.

One minor style nit:

725   if(env->ExceptionOccurred()) {

Please add a space after "if" (yes I see the macros have this wrong too).

No need for updated webrev.

Thanks,
David



Best regards, Matthias


-Original Message-
From: David Holmes 
Sent: Dienstag, 18. Dezember 2018 10:04
To: Baesken, Matthias ; 'hotspot-
d...@openjdk.java.net' ; serviceability-
d...@openjdk.java.net; security-...@openjdk.java.net; JC Beyler

Subject: Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss
corresponding Release

Hi Matthias,

On 18/12/2018 6:52 pm, Baesken, Matthias wrote:

Hi JC  / David,  thanks for the input .

I'm not really sure what you want me to change David,  am I right  that I can

keep   the changes to


src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m
src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c


Yes.


but  adjust

src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp

to also handle theis potential early return

723   IDebugDataSpaces* ptrIDebugDataSpaces = (IDebugDataSpaces*)

env->GetLongField(obj,

   724
ptrIDebugDataSpaces_ID);
   725   CHECK_EXCEPTION_(0);

?


And I think:

   730  THROW_NEW_DEBUGGER_EXCEPTION_("Windbg Error: ReadVirtual
failed!", 0);

as I assume that won't actually return normally.


Also you change seem wrong to me because it will commit the changes

in

the buffer even if we "abort" here:

735   if (bytesRead != numBytes) {
736  return 0;
737   }



The spec says :



https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/function
s.html



ReleaseArrayElements Routines
void ReleaseArrayElements(JNIEnv *env, ArrayType

array, NativeType *elems, jint mode);


A family of functions that informs the VM that the native code no longer

needs access to elems. The elems argument is a pointer derived from array
using the corresponding

GetArrayElements() function. If necessary, this function

copies back all changes made to elems to the original array.


So  shouldn't  I tell the VM , that  the native  code no longer needs access to

bytePtr  before  returning here


735   if (bytesRead != numBytes) {
736  return 0;
   737   }

?


There are two aspects to release:
   1. the actual release (or "I don't need this any more")
   2. committing any changes made back to the original array

This code does:

728   if (ptrIDebugDataSpaces->ReadVirtual((ULONG64) address, (PVOID)
bytePtr,
   729   (ULONG)numBytes, ) !=
S_OK) {

which writes into the array. It then checks if we wrote what we expected:

   735   if (bytesRead != numBytes) {
   736  return 0;
   737   }

If we didn't read what was expected what should happen to the original
array? Should it be left untouched or updated with the unexpected input?
I would say left untouched and that is what the original code did.

If everything succeeds you should do the release with mode 0; but if
taking an error exit the release should use mode JNI_ABORT so no changes
are written back.

Cheers,
David


Best regards, Matthias



-Original Message-
From: David Holmes 
Sent: Dienstag, 18. Dezember 2018 01:20
To: Baesken, Matthias ; 'hotspot-
d...@openjdk.java.net' ; serviceability-
d...@openjdk.java.net; security-...@openjdk.java.net
Subject: Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss
corresponding Release

Correction ...

On 18/12/2018 8:25 am, David Holmes wrote:

Hi Matthias,

On 17/12/2018 6:59 pm, Baesken, Matthias wrote:

Hello, please review the following change.
I noticed that we miss at some places (for example in case of early
returns)
where GetByteArrayElements is used,  the corresponding
ReleaseByteArrayElements  call.

In VirtualMachineImpl.c  I also removed a check for isCopy (is the
returned byte array a copy ?)
because from what I read at





https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/function

s.html



the ReleaseByteArrayElements  routine should always be called.


That's not clear to me. My reading is that you only need to release if
you have changes you need to ensure are written back and that is only
needed if a copy was made.


Jc pointed out to me that if a copy is made you may need to release to
avoid a memory leak. But that is where the mode flags come in - and
again only relevant if a copy was made.

But as per:

https://developer.android.com/training/articles/perf-jni

if a copy is not made and pinning is used (as with the "critical"
variants) then you also need to release to un-pin.

So code should call release, with an appropriate mode, based on whether
isCopy was true**, and whether changed data should be written back.

** 

RFR: JDK-8215568: Refactor SA clhsdb tests to use ClhsdbLauncher

[Jini George]

Hello!

Requesting reviews for:

http://cr.openjdk.java.net/~jgeorge/8215568/webrev.00/

BugID: https://bugs.openjdk.java.net/browse/JDK-8215568

No new failures with the SA tests (hs-tiers 1-5) with these changes. The 
changes here include:


* Refactoring the SA tests which test clhsdb commands to use 
ClhsdbLauncher for uniformity and ease of maintainence.
* Testing for regular expressions with shouldMatch rather than 
shouldContain.

* Minor cleanups.

Thank you,
Jini.

RE: [PATCH] 8214535: Extend JMap to support parallel and incremental heap scanning

[臧琳]

Dear Jc,
   Thanks a lot! I will update the webrev and ask for review separately ☺

BRs,
Lin

From: JC Beyler [mailto:jcbey...@google.com]
Sent: Wednesday, December 19, 2018 11:18 AM
To: 臧琳 
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [PATCH] 8214535: Extend JMap to support parallel and incremental 
heap scanning

Hi Lin,

Done:


1. Add dump to file support for jmap –histo
https://bugs.openjdk.java.net/browse/JDK-8215622


2. Add incremental dump for jmap –histo
 https://bugs.openjdk.java.net/browse/JDK-8215623

3. Add parallel heap iteration for jmap –histo.
https://bugs.openjdk.java.net/browse/JDK-8215624

Is it reasonable ?

Sounded reasonable to me :-)
Jc

Thanks!

BRs,
Lin
From: JC Beyler [mailto:jcbey...@google.com]
Sent: Wednesday, December 19, 2018 12:56 AM
To: 臧琳 mailto:zangl...@jd.com>>
Cc: 
serviceability-dev@openjdk.java.net
Subject: Re: [PATCH] 8214535: Extend JMap to support parallel and incremental 
heap scanning

Hi Lin,

Would it not be easier to make 3 different JBS items that are all enhancements? 
Or perhaps 3 subtasks to the original JDK-8214535? Then you could send out a 
request for review for the first two and we can talk about the third.

I think all three parts can be considered separately (and it seems you do too).

Thanks,
Jc

On Tue, Dec 18, 2018 at 12:58 AM 臧琳 mailto:zangl...@jd.com>> 
wrote:

Hi All,

I am preparing the patch for 
https://bugs.openjdk.java.net/browse/JDK-8214535, and want to get your 
suggestions.



To make the patch easier to review. I plan to make 3 patches as following:

1. one patch for enabling file dump of "jmap 
-histo".(http://cr.openjdk.java.net/~xiaofeya/JDK-8214535/file_dump/webrev.00/webrev/)

2. one patch for incremental dump intermediate data to file of "jmap 
-histo".(http://cr.openjdk.java.net/~xiaofeya/JDK-8214535/incremental/webrev.00/)

3. one patch for parallel iterating heap of "jmap -histo". (WIP)



And the patches for item 1 & 2 are ready. patch for 3 is WIP.

May I ask your help to review these patches?

Thanks.



BRs,

Lin


--

Thanks,
Jc


--

Thanks,
Jc

Re: [PATCH] 8214535: Extend JMap to support parallel and incremental heap scanning

[JC Beyler]

Hi Lin,

Done:

>
> 1. Add dump to file support for jmap –histo
>
https://bugs.openjdk.java.net/browse/JDK-8215622


> 2. Add incremental dump for jmap –histo
>
 https://bugs.openjdk.java.net/browse/JDK-8215623

> 3. Add parallel heap iteration for jmap –histo.
>
https://bugs.openjdk.java.net/browse/JDK-8215624


> Is it reasonable ?
>

Sounded reasonable to me :-)
Jc


> Thanks!
>
>
>
> BRs,
>
> Lin
>
> *From:* JC Beyler [mailto:jcbey...@google.com]
> *Sent:* Wednesday, December 19, 2018 12:56 AM
> *To:* 臧琳 
> *Cc:* serviceability-dev@openjdk.java.net
> *Subject:* Re: [PATCH] 8214535: Extend JMap to support parallel and
> incremental heap scanning
>
>
>
> Hi Lin,
>
>
>
> Would it not be easier to make 3 different JBS items that are all
> enhancements? Or perhaps 3 subtasks to the original JDK-8214535? Then you
> could send out a request for review for the first two and we can talk about
> the third.
>
>
>
> I think all three parts can be considered separately (and it seems you do
> too).
>
>
>
> Thanks,
>
> Jc
>
>
>
> On Tue, Dec 18, 2018 at 12:58 AM 臧琳  wrote:
>
> Hi All,
>
> I am preparing the patch for
> https://bugs.openjdk.java.net/browse/JDK-8214535, and want to get your
> suggestions.
>
>
>
> To make the patch easier to review. I plan to make 3 patches as
> following:
>
> 1. one patch for enabling file dump of "jmap -histo".(
> http://cr.openjdk.java.net/~xiaofeya/JDK-8214535/file_dump/webrev.00/webrev/
> )
>
> 2. one patch for incremental dump intermediate data to file of "jmap
> -histo".(
> http://cr.openjdk.java.net/~xiaofeya/JDK-8214535/incremental/webrev.00/)
>
> 3. one patch for parallel iterating heap of "jmap -histo". (WIP)
>
>
>
> And the patches for item 1 & 2 are ready. patch for 3 is WIP.
>
> May I ask your help to review these patches?
>
> Thanks.
>
>
>
> BRs,
>
> Lin
>
>
>
>
> --
>
>
>
> Thanks,
>
> Jc
>


-- 

Thanks,
Jc

RE: [PATCH] 8214535: Extend JMap to support parallel and incremental heap scanning

[臧琳]

Dear JC,
  Thanks for your comments, I totally like the idea  to have 3 subtasks.
  It seems that I don’t have access of JBS at present. May I ask your help 
for that?
  I think the 3 subtask could be:

1. Add dump to file support for jmap –histo

2. Add incremental dump for jmap –histo

3. Add parallel heap iteration for jmap –histo.
Is it reasonable ?
Thanks!

BRs,
Lin
From: JC Beyler [mailto:jcbey...@google.com]
Sent: Wednesday, December 19, 2018 12:56 AM
To: 臧琳 
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [PATCH] 8214535: Extend JMap to support parallel and incremental 
heap scanning

Hi Lin,

Would it not be easier to make 3 different JBS items that are all enhancements? 
Or perhaps 3 subtasks to the original JDK-8214535? Then you could send out a 
request for review for the first two and we can talk about the third.

I think all three parts can be considered separately (and it seems you do too).

Thanks,
Jc

On Tue, Dec 18, 2018 at 12:58 AM 臧琳 mailto:zangl...@jd.com>> 
wrote:

Hi All,

I am preparing the patch for 
https://bugs.openjdk.java.net/browse/JDK-8214535, and want to get your 
suggestions.



To make the patch easier to review. I plan to make 3 patches as following:

1. one patch for enabling file dump of "jmap 
-histo".(http://cr.openjdk.java.net/~xiaofeya/JDK-8214535/file_dump/webrev.00/webrev/)

2. one patch for incremental dump intermediate data to file of "jmap 
-histo".(http://cr.openjdk.java.net/~xiaofeya/JDK-8214535/incremental/webrev.00/)

3. one patch for parallel iterating heap of "jmap -histo". (WIP)



And the patches for item 1 & 2 are ready. patch for 3 is WIP.

May I ask your help to review these patches?

Thanks.



BRs,

Lin


--

Thanks,
Jc

Re: RFR JDK-8215425: vmTestbase/nsk/jvmti/PopFrame should provide more detailed output

[serguei . spitsyn]

Alex,

Thank you for the update!

It looks good.
There is another instance with incorrect spacing:

121 totRes=doPopFrame(false, Thread.currentThread());


No need in new webrev if you fix the above.

Thanks,
Serguei


On 12/18/18 6:01 PM, Alex Menkov wrote:

Hi Serguei,

Thank you for the review.
Updated webrev:
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.02/

On 12/18/2018 16:49, serguei.spit...@oracle.com wrote:

Hi Alex,

A couple of minor comments.

http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe002.java.frames.html 

http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe004.java.frames.html 



   While you are at these files, could you, fix several originally 
ugly indented comments?


Done.



   Could you, also, fix the incorrect spacing around '=' in the 
popframe004.java:


95 retValue=doPopFrame(true, popFrameClsThr);


Done.

Also fixed comment in popframe004/TestDescription.java (to be in sync 
with comment change in popframe004.java)





   Could you give an idea about the motivation to remove the 
following fragment ?:


This is "test case 2" in popframe004 which I mentioned in the review 
request.
The code is never executed (if it is, this means the test has already 
failed) and I don't have an idea what other case can be tested here.


--alex



167 if (popframe004.popFdone) { // popping has been done
168 if (DEBUG_MODE)
169 out.println("popFrameCls (" + this +
170 "): enter activeMethod() after popping");
171 // test case #2: popping from the current thread
172 if (!popframe004.popF2done) {
173 popframe004.popF2done = true;
174 if (DEBUG_MODE) {
175 out.println("popFrameCls (" + this +
176 "): going to pop a frame from the current thread...");
177 retVal = doPopFrame(3, popFrameClsThr);
178 } else
179 retVal = doPopFrame(2, popFrameClsThr);
180 if (retVal != PASSED)
181 popframe004.totRes = FAILED;
182 }
183 if (DEBUG_MODE)
184 out.println("popFrameCls (" + this +
185 "): leaving activeMethod()...");
186 return;
187 }


Otherwise, it looks good.

Thanks,
Serguei


On 12/18/18 1:37 PM, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8215425
webrev:
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/

The fix
- turns on detailed output for the tests and cleaned related stuff;
- for popframe002 deletes output from run() method as it caused 
unexpected MethodExit event;

- removes "test case 2" in popframe004 test as it's never executed.

--alex

Re: RFR JDK-8215425: vmTestbase/nsk/jvmti/PopFrame should provide more detailed output

[Alex Menkov]

Hi Serguei,

Thank you for the review.
Updated webrev:
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.02/

On 12/18/2018 16:49, serguei.spit...@oracle.com wrote:

Hi Alex,

A couple of minor comments.

http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe002.java.frames.html
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe004.java.frames.html

   While you are at these files, could you, fix several originally ugly 
indented comments?


Done.



   Could you, also, fix the incorrect spacing around '=' in the 
popframe004.java:


95 retValue=doPopFrame(true, popFrameClsThr);


Done.

Also fixed comment in popframe004/TestDescription.java (to be in sync 
with comment change in popframe004.java)





   Could you give an idea about the motivation to remove the following 
fragment ?:


This is "test case 2" in popframe004 which I mentioned in the review 
request.
The code is never executed (if it is, this means the test has already 
failed) and I don't have an idea what other case can be tested here.


--alex



167 if (popframe004.popFdone) { // popping has been done
168 if (DEBUG_MODE)
169 out.println("popFrameCls (" + this +
170 "): enter activeMethod() after popping");
171 // test case #2: popping from the current thread
172 if (!popframe004.popF2done) {
173 popframe004.popF2done = true;
174 if (DEBUG_MODE) {
175 out.println("popFrameCls (" + this +
176 "): going to pop a frame from the current thread...");
177 retVal = doPopFrame(3, popFrameClsThr);
178 } else
179 retVal = doPopFrame(2, popFrameClsThr);
180 if (retVal != PASSED)
181 popframe004.totRes = FAILED;
182 }
183 if (DEBUG_MODE)
184 out.println("popFrameCls (" + this +
185 "): leaving activeMethod()...");
186 return;
187 }


Otherwise, it looks good.

Thanks,
Serguei


On 12/18/18 1:37 PM, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8215425
webrev:
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/

The fix
- turns on detailed output for the tests and cleaned related stuff;
- for popframe002 deletes output from run() method as it caused 
unexpected MethodExit event;

- removes "test case 2" in popframe004 test as it's never executed.

--alex

Re: RFR JDK-8215425: vmTestbase/nsk/jvmti/PopFrame should provide more detailed output

[serguei . spitsyn]

Hi Alex,

A couple of minor comments.

http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe002.java.frames.html
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe004.java.frames.html

  While you are at these files, could you, fix several originally ugly 
indented comments?


  Could you, also, fix the incorrect spacing around '=' in the 
popframe004.java:


95 retValue=doPopFrame(true, popFrameClsThr);


  Could you give an idea about the motivation to remove the following 
fragment ?:


167 if (popframe004.popFdone) { // popping has been done
168 if (DEBUG_MODE)
169 out.println("popFrameCls (" + this +
170 "): enter activeMethod() after popping");
171 // test case #2: popping from the current thread
172 if (!popframe004.popF2done) {
173 popframe004.popF2done = true;
174 if (DEBUG_MODE) {
175 out.println("popFrameCls (" + this +
176 "): going to pop a frame from the current thread...");
177 retVal = doPopFrame(3, popFrameClsThr);
178 } else
179 retVal = doPopFrame(2, popFrameClsThr);
180 if (retVal != PASSED)
181 popframe004.totRes = FAILED;
182 }
183 if (DEBUG_MODE)
184 out.println("popFrameCls (" + this +
185 "): leaving activeMethod()...");
186 return;
187 }


Otherwise, it looks good.

Thanks,
Serguei


On 12/18/18 1:37 PM, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8215425
webrev:
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/

The fix
- turns on detailed output for the tests and cleaned related stuff;
- for popframe002 deletes output from run() method as it caused 
unexpected MethodExit event;

- removes "test case 2" in popframe004 test as it's never executed.

--alex

RFR JDK-8215425: vmTestbase/nsk/jvmti/PopFrame should provide more detailed output

[Alex Menkov]

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8215425
webrev:
http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/

The fix
- turns on detailed output for the tests and cleaned related stuff;
- for popframe002 deletes output from run() method as it caused 
unexpected MethodExit event;

- removes "test case 2" in popframe004 test as it's never executed.

--alex

RE: RFR: [XS] 8215411: some GetByteArrayElements calls miss corresponding Release

[Baesken, Matthias]

Hello, here  is an updated  webrev : 


http://cr.openjdk.java.net/~mbaesken/webrevs/8215411.1/

sawindbg.cpp  has been adjusted .
The exception cases mentioned  now also  call   env->ReleaseByteArrayElements  .

Best regards, Matthias

> -Original Message-
> From: David Holmes 
> Sent: Dienstag, 18. Dezember 2018 10:04
> To: Baesken, Matthias ; 'hotspot-
> d...@openjdk.java.net' ; serviceability-
> d...@openjdk.java.net; security-...@openjdk.java.net; JC Beyler
> 
> Subject: Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss
> corresponding Release
> 
> Hi Matthias,
> 
> On 18/12/2018 6:52 pm, Baesken, Matthias wrote:
> > Hi JC  / David,  thanks for the input .
> >
> > I'm not really sure what you want me to change David,  am I right  that I 
> > can
> keep   the changes to
> >
> > src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m
> > src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c
> 
> Yes.
> 
> > but  adjust
> >
> > src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp
> >
> > to also handle theis potential early return
> >
> > 723   IDebugDataSpaces* ptrIDebugDataSpaces = (IDebugDataSpaces*)
> env->GetLongField(obj,
> >   724
> > ptrIDebugDataSpaces_ID);
> >   725   CHECK_EXCEPTION_(0);
> >
> > ?
> 
> And I think:
> 
>   730  THROW_NEW_DEBUGGER_EXCEPTION_("Windbg Error: ReadVirtual
> failed!", 0);
> 
> as I assume that won't actually return normally.
> 
> >>> Also you change seem wrong to me because it will commit the changes
> in
> >>> the buffer even if we "abort" here:
> >>>
> >>> 735   if (bytesRead != numBytes) {
> >>>736  return 0;
> >>>737   }
> >>>
> >
> > The spec says :
> >
> >
> https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/function
> s.html
> >
> >> ReleaseArrayElements Routines
> >> void ReleaseArrayElements(JNIEnv *env, ArrayType
> array, NativeType *elems, jint mode);
> >>
> >> A family of functions that informs the VM that the native code no longer
> needs access to elems. The elems argument is a pointer derived from array
> using the corresponding
> >> GetArrayElements() function. If necessary, this function
> copies back all changes made to elems to the original array.
> >
> > So  shouldn't  I tell the VM , that  the native  code no longer needs 
> > access to
> bytePtr  before  returning here
> >
> > 735   if (bytesRead != numBytes) {
> > 736  return 0;
> >   737   }
> >
> > ?
> 
> There are two aspects to release:
>   1. the actual release (or "I don't need this any more")
>   2. committing any changes made back to the original array
> 
> This code does:
> 
> 728   if (ptrIDebugDataSpaces->ReadVirtual((ULONG64) address, (PVOID)
> bytePtr,
>   729   (ULONG)numBytes, ) !=
> S_OK) {
> 
> which writes into the array. It then checks if we wrote what we expected:
> 
>   735   if (bytesRead != numBytes) {
>   736  return 0;
>   737   }
> 
> If we didn't read what was expected what should happen to the original
> array? Should it be left untouched or updated with the unexpected input?
> I would say left untouched and that is what the original code did.
> 
> If everything succeeds you should do the release with mode 0; but if
> taking an error exit the release should use mode JNI_ABORT so no changes
> are written back.
> 
> Cheers,
> David
> 
> > Best regards, Matthias
> >
> >
> >> -Original Message-
> >> From: David Holmes 
> >> Sent: Dienstag, 18. Dezember 2018 01:20
> >> To: Baesken, Matthias ; 'hotspot-
> >> d...@openjdk.java.net' ; serviceability-
> >> d...@openjdk.java.net; security-...@openjdk.java.net
> >> Subject: Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss
> >> corresponding Release
> >>
> >> Correction ...
> >>
> >> On 18/12/2018 8:25 am, David Holmes wrote:
> >>> Hi Matthias,
> >>>
> >>> On 17/12/2018 6:59 pm, Baesken, Matthias wrote:
>  Hello, please review the following change.
>  I noticed that we miss at some places (for example in case of early
>  returns)
>  where GetByteArrayElements is used,  the corresponding
>  ReleaseByteArrayElements  call.
> 
>  In VirtualMachineImpl.c  I also removed a check for isCopy (is the
>  returned byte array a copy ?)
>  because from what I read at
> 
> 
> >>
> https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/function
> >> s.html
> 
> 
>  the ReleaseByteArrayElements  routine should always be called.
> >>>
> >>> That's not clear to me. My reading is that you only need to release if
> >>> you have changes you need to ensure are written back and that is only
> >>> needed if a copy was made.
> >>
> >> Jc pointed out to me that if a copy is made you may need to release to
> >> avoid a memory leak. But that is where the mode flags come in - and
> >> again only relevant if a copy was made.
> >>
> >> But as per:
> >>
> >> https://developer.android.com/training/articles/perf-jni
> >>

Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss corresponding Release

[David Holmes]

Hi Matthias,

On 18/12/2018 6:52 pm, Baesken, Matthias wrote:

Hi JC  / David,  thanks for the input .

I'm not really sure what you want me to change David,  am I right  that I can 
keep   the changes to

src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m
src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c


Yes.


but  adjust

src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp

to also handle theis potential early return

723   IDebugDataSpaces* ptrIDebugDataSpaces = (IDebugDataSpaces*) 
env->GetLongField(obj,
  724
ptrIDebugDataSpaces_ID);
  725   CHECK_EXCEPTION_(0);

?


And I think:

 730  THROW_NEW_DEBUGGER_EXCEPTION_("Windbg Error: ReadVirtual 
failed!", 0);


as I assume that won't actually return normally.


Also you change seem wrong to me because it will commit the changes in
the buffer even if we "abort" here:

735   if (bytesRead != numBytes) {
   736  return 0;
   737   }



The spec says :

https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html


ReleaseArrayElements Routines
void ReleaseArrayElements(JNIEnv *env, ArrayType array, 
NativeType *elems, jint mode);

A family of functions that informs the VM that the native code no longer needs 
access to elems. The elems argument is a pointer derived from array using the 
corresponding
GetArrayElements() function. If necessary, this function copies 
back all changes made to elems to the original array.


So  shouldn't  I tell the VM , that  the native  code no longer needs access to 
 bytePtr  before  returning here

735   if (bytesRead != numBytes) {
736  return 0;
  737   }

?


There are two aspects to release:
 1. the actual release (or "I don't need this any more")
 2. committing any changes made back to the original array

This code does:

728   if (ptrIDebugDataSpaces->ReadVirtual((ULONG64) address, (PVOID) 
bytePtr,
 729   (ULONG)numBytes, ) != 
S_OK) {


which writes into the array. It then checks if we wrote what we expected:

 735   if (bytesRead != numBytes) {
 736  return 0;
 737   }

If we didn't read what was expected what should happen to the original 
array? Should it be left untouched or updated with the unexpected input? 
I would say left untouched and that is what the original code did.


If everything succeeds you should do the release with mode 0; but if 
taking an error exit the release should use mode JNI_ABORT so no changes 
are written back.


Cheers,
David


Best regards, Matthias



-Original Message-
From: David Holmes 
Sent: Dienstag, 18. Dezember 2018 01:20
To: Baesken, Matthias ; 'hotspot-
d...@openjdk.java.net' ; serviceability-
d...@openjdk.java.net; security-...@openjdk.java.net
Subject: Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss
corresponding Release

Correction ...

On 18/12/2018 8:25 am, David Holmes wrote:

Hi Matthias,

On 17/12/2018 6:59 pm, Baesken, Matthias wrote:

Hello, please review the following change.
I noticed that we miss at some places (for example in case of early
returns)
where GetByteArrayElements is used,  the corresponding
ReleaseByteArrayElements  call.

In VirtualMachineImpl.c  I also removed a check for isCopy (is the
returned byte array a copy ?)
because from what I read at



https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/function
s.html



the ReleaseByteArrayElements  routine should always be called.


That's not clear to me. My reading is that you only need to release if
you have changes you need to ensure are written back and that is only
needed if a copy was made.


Jc pointed out to me that if a copy is made you may need to release to
avoid a memory leak. But that is where the mode flags come in - and
again only relevant if a copy was made.

But as per:

https://developer.android.com/training/articles/perf-jni

if a copy is not made and pinning is used (as with the "critical"
variants) then you also need to release to un-pin.

So code should call release, with an appropriate mode, based on whether
isCopy was true**, and whether changed data should be written back.

** mode is ignored if not working on a copy so you can just set it
assuming a copy was made.

Note that the hotspot implementation always makes a copy for
GextXXXArrayElements, and the ReleaseXXXArrayElements knows this and
so
will always free the buffer that is passed in (other than for mode
JNI_COMMIT of course).

Cheers,
David
-


That said, the change seem semantically correct and harmless in this case.
---

src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp

AFAICS there are still multiple exit paths via CHECK_EXCEPTION_(0) and
THROW_NEW_DEBUGGER_EXCEPTION_ that won't release the buffer.

Also you change seem wrong to me because it will commit the changes in
the buffer even if we "abort" here:

735   if (bytesRead != numBytes) {
   736  return 0;
   737   }

so it seems to me if you really want to 

[PATCH] 8214535: Extend JMap to support parallel and incremental heap scanning

[臧琳]

Hi All,

I am preparing the patch for 
https://bugs.openjdk.java.net/browse/JDK-8214535, and want to get your 
suggestions.


To make the patch easier to review. I plan to make 3 patches as following:

1. one patch for enabling file dump of "jmap 
-histo".(http://cr.openjdk.java.net/~xiaofeya/JDK-8214535/file_dump/webrev.00/webrev/)

2. one patch for incremental dump intermediate data to file of "jmap 
-histo".(http://cr.openjdk.java.net/~xiaofeya/JDK-8214535/incremental/webrev.00/)

3. one patch for parallel iterating heap of "jmap -histo". (WIP)


And the patches for item 1 & 2 are ready. patch for 3 is WIP.

May I ask your help to review these patches?

Thanks.


BRs,

Lin

RE: RFR: [XS] 8215411: some GetByteArrayElements calls miss corresponding Release

[Baesken, Matthias]

Hi JC  / David,  thanks for the input .

I'm not really sure what you want me to change David,  am I right  that I can 
keep   the changes to

src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m
src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c

but  adjust  

src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp

to also handle theis potential early return 

723   IDebugDataSpaces* ptrIDebugDataSpaces = (IDebugDataSpaces*) 
env->GetLongField(obj,
 724
ptrIDebugDataSpaces_ID);
 725   CHECK_EXCEPTION_(0);

?

> > Also you change seem wrong to me because it will commit the changes in
> > the buffer even if we "abort" here:
> >
> > 735   if (bytesRead != numBytes) {
> >   736  return 0;
> >   737   }
> >

The spec says :

https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html

>ReleaseArrayElements Routines
>void ReleaseArrayElements(JNIEnv *env, ArrayType array, 
>NativeType *elems, jint mode);
>
>A family of functions that informs the VM that the native code no longer needs 
>access to elems. The elems argument is a pointer derived from array using the 
>corresponding 
> GetArrayElements() function. If necessary, this function 
> copies back all changes made to elems to the original array.

So  shouldn't  I tell the VM , that  the native  code no longer needs access to 
 bytePtr  before  returning here 

735   if (bytesRead != numBytes) {
736  return 0;
 737   }

?

Best regards, Matthias


> -Original Message-
> From: David Holmes 
> Sent: Dienstag, 18. Dezember 2018 01:20
> To: Baesken, Matthias ; 'hotspot-
> d...@openjdk.java.net' ; serviceability-
> d...@openjdk.java.net; security-...@openjdk.java.net
> Subject: Re: RFR: [XS] 8215411: some GetByteArrayElements calls miss
> corresponding Release
> 
> Correction ...
> 
> On 18/12/2018 8:25 am, David Holmes wrote:
> > Hi Matthias,
> >
> > On 17/12/2018 6:59 pm, Baesken, Matthias wrote:
> >> Hello, please review the following change.
> >> I noticed that we miss at some places (for example in case of early
> >> returns)
> >> where GetByteArrayElements is used,  the corresponding
> >> ReleaseByteArrayElements  call.
> >>
> >> In VirtualMachineImpl.c  I also removed a check for isCopy (is the
> >> returned byte array a copy ?)
> >> because from what I read at
> >>
> >>
> https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/function
> s.html
> >>
> >>
> >> the ReleaseByteArrayElements  routine should always be called.
> >
> > That's not clear to me. My reading is that you only need to release if
> > you have changes you need to ensure are written back and that is only
> > needed if a copy was made.
> 
> Jc pointed out to me that if a copy is made you may need to release to
> avoid a memory leak. But that is where the mode flags come in - and
> again only relevant if a copy was made.
> 
> But as per:
> 
> https://developer.android.com/training/articles/perf-jni
> 
> if a copy is not made and pinning is used (as with the "critical"
> variants) then you also need to release to un-pin.
> 
> So code should call release, with an appropriate mode, based on whether
> isCopy was true**, and whether changed data should be written back.
> 
> ** mode is ignored if not working on a copy so you can just set it
> assuming a copy was made.
> 
> Note that the hotspot implementation always makes a copy for
> GextXXXArrayElements, and the ReleaseXXXArrayElements knows this and
> so
> will always free the buffer that is passed in (other than for mode
> JNI_COMMIT of course).
> 
> Cheers,
> David
> -
> 
> > That said, the change seem semantically correct and harmless in this case.
> > ---
> >
> > src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp
> >
> > AFAICS there are still multiple exit paths via CHECK_EXCEPTION_(0) and
> > THROW_NEW_DEBUGGER_EXCEPTION_ that won't release the buffer.
> >
> > Also you change seem wrong to me because it will commit the changes in
> > the buffer even if we "abort" here:
> >
> > 735   if (bytesRead != numBytes) {
> >   736  return 0;
> >   737   }
> >
> > so it seems to me if you really want to release on all paths then the
> > error paths should use a mode of JNI_ABORT and only the success path
> > should use mode 0.
> >
> > ---
> >
> >   src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m
> >
> > This change seems okay, though again potentially not necessary - as we
> > never commit any changes.
> >
> > Thanks,
> > David
> > -
> >
> >>
> >> bug/webrev :
> >>
> >> https://bugs.openjdk.java.net/browse/JDK-8215411
> >>
> >> http://cr.openjdk.java.net/~mbaesken/webrevs/8215411.0/
> >>
> >>
> >> Thanks, Matthias
> >>