Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v6]

[Dean Long]

On Fri, 6 May 2022 09:40:37 GMT, Jaroslav Bachorik  
wrote:

>> A gist of the fix is to allow relaxed special handling of code blob lookup 
>> when done for ASGCT. 
>> 
>> Currently, a guarantee will fail when we happen to hit a zombie method which 
>> is still on stack. While this would indicate a serious error for the normal 
>> execution flow, in case of ASGCT being in progress when the executing thread 
>> can be expected at any possible method this is something which may happen 
>> and we really should not take the profiled JVM down due to it.
>> 
>> 
>> Unfortunately, I am not able to create a simple reproducer for the crash 
>> other that testing in our production where the crash is happening 
>> sporadically.
>> However, thanks to @parttimenerd and his [ASGCT stress 
>> test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be 
>> reproduced quite reliably.
>> 
>> 
>> 
>> _Note: This is a followup PR for #8061_
>
> Jaroslav Bachorik has refreshed the contents of this pull request, and 
> previous commits have been removed. The incremental views will show 
> differences compared to the previous content of the PR. The pull request 
> contains one new commit since the last revision:
> 
>   Move 'in_asgct' flag to JavaThread

David, your example has an extra || result->is_zombie(), but otherwise this 
sounds perfect.

-

PR: https://git.openjdk.java.net/jdk/pull/8549

Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v6]

[Dean Long]

On Fri, 6 May 2022 09:40:37 GMT, Jaroslav Bachorik  
wrote:

>> A gist of the fix is to allow relaxed special handling of code blob lookup 
>> when done for ASGCT. 
>> 
>> Currently, a guarantee will fail when we happen to hit a zombie method which 
>> is still on stack. While this would indicate a serious error for the normal 
>> execution flow, in case of ASGCT being in progress when the executing thread 
>> can be expected at any possible method this is something which may happen 
>> and we really should not take the profiled JVM down due to it.
>> 
>> 
>> Unfortunately, I am not able to create a simple reproducer for the crash 
>> other that testing in our production where the crash is happening 
>> sporadically.
>> However, thanks to @parttimenerd and his [ASGCT stress 
>> test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be 
>> reproduced quite reliably.
>> 
>> 
>> 
>> _Note: This is a followup PR for #8061_
>
> Jaroslav Bachorik has refreshed the contents of this pull request, and 
> previous commits have been removed. The incremental views will show 
> differences compared to the previous content of the PR. The pull request 
> contains one new commit since the last revision:
> 
>   Move 'in_asgct' flag to JavaThread

It seems like you should be able to mitigate the extra overhead by only doing 
that extra work when we actually see a zombie method, which should be rare.

-

PR: https://git.openjdk.java.net/jdk/pull/8549

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Dean Long]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

The compiler changes look good to me.

Marked as reviewed by dlong (Reviewer).

-

Marked as reviewed by dlong (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Dean Long]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

Marked as reviewed by dlong (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8218885: Restore pop_frame and force_early_return functionality for Graal

[Dean Long]

On Wed, 22 Sep 2021 05:40:40 GMT, Tom Rodriguez  wrote:

> This logic no longer seems to be necessary since the adjustCompilationLevel 
> callback has been removed.

Marked as reviewed by dlong (Reviewer).

Make sure to test with -Xcomp.

-

PR: https://git.openjdk.java.net/jdk/pull/5625

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Dean Long]

On Fri, 9 Apr 2021 16:54:51 GMT, Ioi Lam  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> LGTM. Just a small nit.

@iklam
I thought the fingerprint code was also used by CDS.

-

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop

[Dean Long]

On Thu, 29 Oct 2020 12:44:58 GMT, Erik Österlund  wrote:

> The imasm::remove_activation() call does not deal with safepoints very well. 
> However, when the MethodExit JVMTI event is being called, we call into the 
> runtime in the middle of remove_activation(). If the value being returned is 
> an object type, then the top-of-stack contains the oop. However, the GC does 
> not traverse said oop in any oop map, because it is simply not expected that 
> we safepoint in the middle of remove_activation().
> 
> The JvmtiExport::post_method_exit() function we end up calling, reads the 
> top-of-stack oop, and puts it in a handle. Then it calls JVMTI callbacks, 
> that eventually call Java and a bunch of stuff that safepoints. So after the 
> JVMTI callback, we can expect the top-of-stack oop to be broken. 
> Unfortunately, when we continue, we therefore end up returning a broken oop.
> 
> Notably, the fact that InterpreterRuntime::post_method_exit is a JRT_ENTRY, 
> is wrong, as we can safepoint on the way back to Java, which will break the 
> return oop in a similar way. So this patch makes it a JRT_BLOCK_ENTRY, moving 
> the transition to VM and back, into a block of code that is protected against 
> GC. Before the JRT_BLOCK is called, we stash away the return oop, and after 
> the JRT_BLOCK_END, we restore the top-of-stack oop. In the path when 
> InterpreterRuntime::post_method_exit is called when throwing an exception, we 
> don't have the same problem of retaining an oop result, and hence the 
> JRT_BLOCK/JRT_BLOCK_END section is not performed in this case; the logic is 
> the same as before for this path. 
> 
> This is a JVMTI bug that has probably been around for a long time. It crashes 
> with all GCs, but was discovered recently after concurrent stack processing, 
> as StefanK has been running better GC stressing code in JVMTI, and the bug 
> reproduced more easily with concurrent stack processing, as the timings were 
> a bit different. The following reproducer failed pretty much 100% of the time:
> while true; do make test JTREG="RETAIN=all" 
> TEST=test/hotspot/jtreg/vmTestbase/nsk/jdi/MethodExitEvent/returnValue/returnValue003/returnValue003.java
>  TEST_OPTS_JAVA_OPTIONS="-XX:+UseZGC -Xmx2g -XX:ZCollectionInterval=0.0001 
> -XX:ZFragmentationLimit=0.01 -XX:+VerifyOops -XX:+ZVerifyViews -Xint" ; done 
> 
> With my fix I can run this repeatedly without any more failures. I have also 
> sanity checked the patch by running tier 1-5, so that it does not introduces 
> any new issues on its own. I have also used Stefan's nice external GC 
> stressing with jcmd technique that was used to trigger crashes with other 
> GCs, to make sure said crashes no longer reproduce either.

Marked as reviewed by dlong (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/930

Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop

[Dean Long]

On Fri, 30 Oct 2020 06:56:13 GMT, Richard Reingruber  wrote:

>> Changes requested by coleenp (Reviewer).
>
> Hi Erik,
> 
> is it possible for GC to mistake a primitive value for a reference when 
> posting the exit event?
> 
> My understanding is: we are at a random bci of a method that is forced to 
> return early. The expression stack is emptied and the return value is pushed 
> on the expression stack then we call into the interpreter runtime to post the 
> JVMTI method exit event during which we come to a safepoint for GC. The oop 
> map for the bci does not cover this forced early return and if the return 
> value is an object then the reference pushed on the expression stack before 
> is not updated by GC. With your fix the value is updated if it is a reference.
> 
> If this is correct then to me it appears as if GC can also crash because the 
> oop map for the random bci tells there has to be a reference at the stack 
> position of the return value if it actually is a primitive value.

I think you've discovered JDK-6449023.

-

PR: https://git.openjdk.java.net/jdk/pull/930

Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods

[Dean Long]

On 5/31/20 11:16 PM, serguei.spit...@oracle.com wrote:

Hi Dean,

To check the is_old as you suggest the target method has to be passed
to the cache_jvmti_state() as argument. Is it what you are suggesting?


I believe you can use use _task->method()->is_old(), as the ciEnv 
already has the task.



Just want to make sure I understand you correctly.

The cache_jvmti_state() and cache_dtrace_flags() are called in the
CompileBroker::init_compiler_runtime() for a ciEnv with the NULL 
CompileTask

which looks unnecessary (or I don't understand it):

bool CompileBroker::init_compiler_runtime() {
  CompilerThread* thread = CompilerThread::current();
  . . .
    ciEnv ci_env((CompileTask*)NULL);
    // Cache Jvmti state
    ci_env.cache_jvmti_state();
    // Cache DTrace flags
    ci_env.cache_dtrace_flags();



These calls look unnecessary to me, as the ci_env will cache these again 
before compiling a method.
I suggest removing these calls.  We should make sure the cache fields 
are initialized to sane values

in the ciEnv ctor.


The JVMCI has a separate implementation for ciEnv which is jvmciEnv and
its own set of cache_jvmti_state() and jvmti_state_changed() functions.
Both are not called in the JVMCI case.
So, these checks look as broken in JVMCI now.

JVMCI is in better shape, because it doesn't transition out of 
_thread_in_vm state,

but yes it needs similar changes.

Not sure, I have enough compiler knowledge to fix this at this stage 
of release.

Would it better to file a separate hotspot/compiler RFE targeted to 16?
It can be assigned to me if it helps.



This is a P3 so I believe we have time to fix it for 15.  Please go 
ahead and let's see if
we can get it in.  I can help with the JVMCI changes if they are not 
straightforward.


dl


Thanks,
Serguei


On 5/28/20 10:54, Dean Long wrote:
Sure, you could just have cache_jvmti_state() return a boolean to 
bail out immediately for is_old.


dl

On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote:

Hi Dean,

Thank you for looking at this!
Okay. Let me check what cab be done in this direction.
There is no point to cache is_old. The compilation has to bail out 
if it is discovered to be true.


Thanks,
Serguei


On 5/28/20 00:59, Dean Long wrote:
This seems OK as long as the memory barriers in the thread state 
transitions prevent the C++ compiler from doing something like 
reading is_old before reading redefinition_count.  I would feel 
better if both JVMCI and C1/C2 cached is_old and redefinition_count 
at the same time (making sure to be in the _thread_in_vm state), 
then bail out based on the cached value of is_old.


dl

On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote:

On 5/25/20 23:39, serguei.spit...@oracle.com wrote:

Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/


Summary:
  The Kitchensink stress test with the Instrumentation module 
enabled does
  a lot of class retransformations in parallel with all other 
stressing.

  It provokes the assert at the compiled code installation time:
    assert(!method->is_old()) failed: Should not be installing 
old methods


  The problem is that the 
CompileBroker::invoke_compiler_on_method in C2 version
  (non-JVMCI tiered compilation) is missing the check that exists 
in the JVMCI

  part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150   failure_reason = "redefined method";
2151   retry_message = "not retryable";
2152   compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }

   The fix is to add this check.


Sorry, forgot to explain one thing.
Compiler code has a special mechanism to ensure the JVMTI class 
redefinition did
not happen while the method was compiled, so all the assumptions 
remain correct.

   2190 // Cache Jvmti state
   2191 ci_env.cache_jvmti_state();
Part of this is a check that the value of 
JvmtiExport::redefinition_count() is

cached in ciEnv variable: _jvmti_redefinition_count.
The JvmtiExport::redefinition_count() value change means a class 
redefinition

happened which also implies some of methods may become old.
However, the method being compiled can be already old at the point 
where the
redefinition counter is cached, so the redefinition counter check 
does not help much.


Thanks,
Serguei


Testing:
   Ran Kitchensink test with the Instrumentation module enabled in mach5
   multiple times for 100 times. Without the fix the test normally fails
   a couple of times in 200 runs. It does not fail with the fix anymore.
   Will also submit hs tiers1-5.

Thanks,
Serguei

Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods

[Dean Long]

Sure, you could just have cache_jvmti_state() return a boolean to bail 
out immediately for is_old.


dl

On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote:

Hi Dean,

Thank you for looking at this!
Okay. Let me check what cab be done in this direction.
There is no point to cache is_old. The compilation has to bail out if 
it is discovered to be true.


Thanks,
Serguei


On 5/28/20 00:59, Dean Long wrote:
This seems OK as long as the memory barriers in the thread state 
transitions prevent the C++ compiler from doing something like 
reading is_old before reading redefinition_count.  I would feel 
better if both JVMCI and C1/C2 cached is_old and redefinition_count 
at the same time (making sure to be in the _thread_in_vm state), then 
bail out based on the cached value of is_old.


dl

On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote:

On 5/25/20 23:39, serguei.spit...@oracle.com wrote:

Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/


Summary:
  The Kitchensink stress test with the Instrumentation module 
enabled does
  a lot of class retransformations in parallel with all other 
stressing.

  It provokes the assert at the compiled code installation time:
    assert(!method->is_old()) failed: Should not be installing old 
methods


  The problem is that the CompileBroker::invoke_compiler_on_method 
in C2 version
  (non-JVMCI tiered compilation) is missing the check that exists 
in the JVMCI

  part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150   failure_reason = "redefined method";
2151   retry_message = "not retryable";
2152   compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }

   The fix is to add this check.


Sorry, forgot to explain one thing.
Compiler code has a special mechanism to ensure the JVMTI class 
redefinition did
not happen while the method was compiled, so all the assumptions 
remain correct.

   2190 // Cache Jvmti state
   2191 ci_env.cache_jvmti_state();
Part of this is a check that the value of 
JvmtiExport::redefinition_count() is

cached in ciEnv variable: _jvmti_redefinition_count.
The JvmtiExport::redefinition_count() value change means a class 
redefinition

happened which also implies some of methods may become old.
However, the method being compiled can be already old at the point 
where the
redefinition counter is cached, so the redefinition counter check 
does not help much.


Thanks,
Serguei


Testing:
   Ran Kitchensink test with the Instrumentation module enabled in mach5
   multiple times for 100 times. Without the fix the test normally fails
   a couple of times in 200 runs. It does not fail with the fix anymore.
   Will also submit hs tiers1-5.

Thanks,
Serguei

Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods

[Dean Long]

This seems OK as long as the memory barriers in the thread state 
transitions prevent the C++ compiler from doing something like reading 
is_old before reading redefinition_count.  I would feel better if both 
JVMCI and C1/C2 cached is_old and redefinition_count at the same time 
(making sure to be in the _thread_in_vm state), then bail out based on 
the cached value of is_old.


dl

On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote:

On 5/25/20 23:39, serguei.spit...@oracle.com wrote:

Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/


Summary:
  The Kitchensink stress test with the Instrumentation module enabled 
does

  a lot of class retransformations in parallel with all other stressing.
  It provokes the assert at the compiled code installation time:
    assert(!method->is_old()) failed: Should not be installing old 
methods


  The problem is that the CompileBroker::invoke_compiler_on_method in 
C2 version
  (non-JVMCI tiered compilation) is missing the check that exists in 
the JVMCI

  part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150   failure_reason = "redefined method";
2151   retry_message = "not retryable";
2152   compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }

   The fix is to add this check.


Sorry, forgot to explain one thing.
Compiler code has a special mechanism to ensure the JVMTI class 
redefinition did
not happen while the method was compiled, so all the assumptions 
remain correct.

   2190 // Cache Jvmti state
   2191 ci_env.cache_jvmti_state();
Part of this is a check that the value of 
JvmtiExport::redefinition_count() is

cached in ciEnv variable: _jvmti_redefinition_count.
The JvmtiExport::redefinition_count() value change means a class 
redefinition

happened which also implies some of methods may become old.
However, the method being compiled can be already old at the point 
where the
redefinition counter is cached, so the redefinition counter check does 
not help much.


Thanks,
Serguei


Testing:
   Ran Kitchensink test with the Instrumentation module enabled in mach5
   multiple times for 100 times. Without the fix the test normally fails
   a couple of times in 200 runs. It does not fail with the fix anymore.
   Will also submit hs tiers1-5.

Thanks,
Serguei

Re: (S) 8237750: Load libzip.so only if necessary

[Dean Long]

Hi Ioi.  I think you're right.  Good catch.

dl

On 5/3/20 10:01 PM, Ioi Lam wrote:

Hi Dean,

The code uses double-checked locking so we can be thread-safe when 
loading the zip library, while avoiding using the MutexLock every time:


https://en.wikipedia.org/wiki/Double-checked_locking

To work correctly on modern memory architectures, we need the memory 
barriers:


 977 void ClassLoader::load_zip_library() {
 978   if (Atomic::load_acquire(_loaded) == 0) {
 979 MutexLocker locker(Zip_lock, Monitor::_no_safepoint_check_flag);
 980 if (libzip_loaded == 0) {
 981   load_zip_library0();
 982   Atomic::release_store(_loaded, 1);
 983 }
 984   }
 985 }

(Atomic::load_acquire is much cheaper than MutexLocker).

If we read Crc32 without memory barriers, as in the original code, in 
two concurrent threads, I think there may be a chance for one of the 
threads to read an invalid value of Crc32.


int ClassLoader::crc32(int crc, const char* buf, int len) {
  return (*Crc32)(crc, (const jbyte*)buf, len);
}

Thanks
- Ioi


On 5/1/20 8:00 PM, Yumin Qi wrote:

Dean,

  I have updated to use MutexLocker instead at same link: 
http://cr.openjdk.java.net/~minqi/8237750/webrev-01/


  Tested locally, passed jtreg/runtime.

Thanks
Yumin

On 5/1/20 4:24 PM, Dean Long wrote:

OK, I didn't realize compute_fingerprint is using ZIP_CRC32.

dl

On 5/1/20 2:42 PM, Yumin Qi wrote:

Hi, Dean
  Thanks for the review. I will try MutextLocker, for AOT, I think 
currently the tests are not using CDS then it will load classes 
from stream, that will use libzip's Crc32.
  I will check for AOT to see if it really loads libzip with the 
patch. For test compiler/aot/DeoptimizationTest.java:


#0  ClassLoader::load_zip_library () at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:978 

#1  0x757d4693 in ClassLoader::crc32 (crc=0, 
buf=0x70244ee0 "\312\376\272\276", len=1888) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1028
#2  0x757cef5d in ClassFileStream::compute_fingerprint 
(this=0x70245640) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileStream.cpp:80
#3  0x757c40ed in ClassFileParser::create_instance_klass 
(this=0x77fc6fc0, changed_by_loadhook=false, cl_inst_info=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileParser.cpp:5630
#4  0x75dea647 in KlassFactory::create_from_stream 
(stream=0x70245640, name=0x740550f0, 
loader_data=0x7022dbc0, cl_info=..., 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/klassFactory.cpp:207
#5  0x757d53e4 in ClassLoader::load_class 
(name=0x740550f0, search_append_only=false, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1285
#6  0x76234fcf in SystemDictionary::load_instance_class 
(class_name=0x740550f0, class_loader=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1550
#7  0x76232d0a in 
SystemDictionary::resolve_instance_class_or_null 
(name=0x740550f0, class_loader=..., protection_domain=..., 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:882
#8  0x7623137e in 
SystemDictionary::resolve_instance_class_or_null_helper 
(class_name=0x740550f0, class_loader=..., 
protection_domain=..., __the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:302
#9  0x7623124c in SystemDictionary::resolve_or_null 
(class_name=0x740550f0, class_loader=..., 
protection_domain=..., __the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:285
#10 0x76230f57 in SystemDictionary::resolve_or_fail 
(class_name=0x740550f0, class_loader=..., 
protection_domain=..., throw_error=true, 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:233
#11 0x762311eb in SystemDictionary::resolve_or_fail 
(class_name=0x740550f0, throw_error=true, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:275
#12 0x76236722 in SystemDictionary::resolve_wk_klass 
(id=SystemDictionary::Object_klass_knum, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2029
#13 0x7623681e in 
SystemDictionary::resolve_wk_klasses_until 
(limit_id=SystemDictionary::Cloneable_klass_knum, 
start_id=@0x77fc79d4: SystemDictionary::Object_klass_knum, 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2039
#14 0x7623ac01 in 
SystemDictionary::resolve_wk_klasses_through 
(end_id=System

Re: (S) 8237750: Load libzip.so only if necessary

[Dean Long]

In ClassLoader::crc32, instead of checking if the library is loaded 
every time, how about initializing the Crc32 function pointer to a 
function that calls load_zip_library()?

Something like:

static jint initial_Crc32(jint crc, const jbyte *buf, jint len) {
    load_zip_library();
    assert(Crc32 != initial_Crc32, "load_zip_library did not update 
Crc32");

    return crc32(crc, buf, len); // ZIP_CRC32
}

static Crc32_t   Crc32  = initial_Crc32;

I also suggest putting

ClassLoader::crc32 in classLoader.hpp so it can be inlined.


dl

On 5/1/20 8:00 PM, Yumin Qi wrote:

Dean,

  I have updated to use MutexLocker instead at same link: 
http://cr.openjdk.java.net/~minqi/8237750/webrev-01/


  Tested locally, passed jtreg/runtime.

Thanks
Yumin

On 5/1/20 4:24 PM, Dean Long wrote:

OK, I didn't realize compute_fingerprint is using ZIP_CRC32.

dl

On 5/1/20 2:42 PM, Yumin Qi wrote:

Hi, Dean
  Thanks for the review. I will try MutextLocker, for AOT, I think 
currently the tests are not using CDS then it will load classes from 
stream, that will use libzip's Crc32.
  I will check for AOT to see if it really loads libzip with the 
patch. For test compiler/aot/DeoptimizationTest.java:


#0  ClassLoader::load_zip_library () at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:978 

#1  0x757d4693 in ClassLoader::crc32 (crc=0, 
buf=0x70244ee0 "\312\376\272\276", len=1888) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1028
#2  0x757cef5d in ClassFileStream::compute_fingerprint 
(this=0x70245640) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileStream.cpp:80
#3  0x757c40ed in ClassFileParser::create_instance_klass 
(this=0x77fc6fc0, changed_by_loadhook=false, cl_inst_info=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileParser.cpp:5630
#4  0x75dea647 in KlassFactory::create_from_stream 
(stream=0x70245640, name=0x740550f0, 
loader_data=0x7022dbc0, cl_info=..., __the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/klassFactory.cpp:207
#5  0x757d53e4 in ClassLoader::load_class 
(name=0x740550f0, search_append_only=false, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1285
#6  0x76234fcf in SystemDictionary::load_instance_class 
(class_name=0x740550f0, class_loader=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1550
#7  0x76232d0a in 
SystemDictionary::resolve_instance_class_or_null 
(name=0x740550f0, class_loader=..., protection_domain=..., 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:882
#8  0x7623137e in 
SystemDictionary::resolve_instance_class_or_null_helper 
(class_name=0x740550f0, class_loader=..., protection_domain=..., 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:302
#9  0x7623124c in SystemDictionary::resolve_or_null 
(class_name=0x740550f0, class_loader=..., protection_domain=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:285
#10 0x76230f57 in SystemDictionary::resolve_or_fail 
(class_name=0x740550f0, class_loader=..., protection_domain=..., 
throw_error=true, __the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:233
#11 0x762311eb in SystemDictionary::resolve_or_fail 
(class_name=0x740550f0, throw_error=true, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:275
#12 0x76236722 in SystemDictionary::resolve_wk_klass 
(id=SystemDictionary::Object_klass_knum, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2029
#13 0x7623681e in SystemDictionary::resolve_wk_klasses_until 
(limit_id=SystemDictionary::Cloneable_klass_knum, 
start_id=@0x77fc79d4: SystemDictionary::Object_klass_knum, 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2039
#14 0x7623ac01 in 
SystemDictionary::resolve_wk_klasses_through 
(end_id=SystemDictionary::Class_klass_knum, 
start_id=@0x77fc79d4: SystemDictionary::Object_klass_knum, 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.hpp:418
#15 0x762369b0 in 
SystemDictionary::resolve_well_known_classes 
(__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2082
#16 0x76236517 in SystemDictionary::initialize 
(__the_thread__=0x7ff

Re: (S) 8237750: Load libzip.so only if necessary

[Dean Long]

fff5bf5677 in JNI_CreateJavaVM (vm=0x77fc7ea8, 
penv=0x77fc7eb0, args=0x77fc7e50) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/prims/jni.cpp:3872
#23 0x77bca4c9 in InitializeJVM (pvm=0x77fc7ea8, 
penv=0x77fc7eb0, ifn=0x77fc7f00) at 
/home/minqi/ws/jdk15/jdk/src/java.base/share/native/libjli/java.c:1538
#24 0x77bc70b5 in JavaMain (_args=0x7fffab10) at 
/home/minqi/ws/jdk15/jdk/src/java.base/share/native/libjli/java.c:417
#25 0x77bceb5f in ThreadJavaMain (args=0x7fffab10) at 
/home/minqi/ws/jdk15/jdk/src/java.base/unix/native/libjli/java_md_solinux.c:734 

#26 0x771c56ba in start_thread (arg=0x77fc8700) at 
pthread_create.c:333
#27 0x7790041d in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:109


This is not with CDS.

Thanks
Yumin

On 5/1/20 2:11 PM, Dean Long wrote:
It looks like Zip_lock could be a MutexLocker instead of a 
MonitorLocker.


dl

On 5/1/20 10:24 AM, Yumin Qi wrote:

Hi, please review:
bug 8237750: https://bugs.openjdk.java.net/browse/JDK-8237750
webrev: http://cr.openjdk.java.net/~minqi/8237750/webrev-01/
Summary:
  zip library is loaded unconditionally at start up but it is only 
needed when

  1) bootclasspath contains zip or
  2) UseAOT enabled or
  3) VerifySharedArchive turned on or
  4) CDS archives custom loaded classes
   If none of above in java application, it is not necessary to have 
zip library loaded.


  Solution by loading zip library on demand when needed.

Performance for java -Xint version:

Results of " perf stat -r 50 bin/java -Xshare:on 
-XX:SharedArchiveFile=jdk2.jsa -Xint -version "
   1: 59611556    59450206 (-161350)  - 39.799 
40.274 (  0.475)    ++
   2: 59602708    59425234 (-177474)  - 40.591 
41.183 (  0.592)    ++
   3: 59579718    59441272 (-138446)    40.777 
40.471 ( -0.306)  -
   4: 59584882    59410155 (-174727)  - 40.824 
40.233 ( -0.591)  --
   5: 59590998    59447252 (-143746)    40.400 
40.493 (  0.093)
   6: 59589523    59441934 (-147589)    40.475 
40.064 ( -0.411)  --
   7: 59581820    59413612 (-168208)  - 39.763 
40.077 (  0.314) +
   8: 59593678    59418738 (-174940)  - 40.912 
39.724 ( -1.188)  -
   9: 59573058    59412554 (-160504)  - 40.126 
40.033 ( -0.093)
  10: 59591469    59419291 (-172178)  - 40.731 
40.689 ( -0.042)


  59589940    59428022 (-161917)  - 40.438 
40.322 ( -0.116)

instr delta =  -161917    -0.2717%
time  delta =   -0.116 ms -0.2859%

Tests: hs-tier1-4.
Due to zip library not loaded at default, I removed 'zip' from pmap 
list in test case: 
*test/hotspot/jtreg/serviceability/sa/ClhsdbPmap.java


**
* Thanks
Yumin

Re: (S) 8237750: Load libzip.so only if necessary

[Dean Long]

It looks like Zip_lock could be a MutexLocker instead of a MonitorLocker.

dl

On 5/1/20 10:24 AM, Yumin Qi wrote:

Hi, please review:
bug 8237750: https://bugs.openjdk.java.net/browse/JDK-8237750
webrev: http://cr.openjdk.java.net/~minqi/8237750/webrev-01/
Summary:
  zip library is loaded unconditionally at start up but it is only 
needed when

  1) bootclasspath contains zip or
  2) UseAOT enabled or
  3) VerifySharedArchive turned on or
  4) CDS archives custom loaded classes
   If none of above in java application, it is not necessary to have 
zip library loaded.


  Solution by loading zip library on demand when needed.

Performance for java -Xint version:

Results of " perf stat -r 50 bin/java -Xshare:on 
-XX:SharedArchiveFile=jdk2.jsa -Xint -version "
   1: 59611556    59450206 (-161350)  - 39.799 40.274 
(  0.475)    ++
   2: 59602708    59425234 (-177474)  - 40.591 41.183 
(  0.592)    ++
   3: 59579718    59441272 (-138446)    40.777 40.471 
( -0.306)  -
   4: 59584882    59410155 (-174727)  - 40.824 40.233 
( -0.591)  --
   5: 59590998    59447252 (-143746)    40.400 40.493 
(  0.093)
   6: 59589523    59441934 (-147589)    40.475 40.064 
( -0.411)  --
   7: 59581820    59413612 (-168208)  - 39.763 40.077 
(  0.314) +
   8: 59593678    59418738 (-174940)  - 40.912 39.724 
( -1.188)  -
   9: 59573058    59412554 (-160504)  - 40.126 40.033 
( -0.093)
  10: 59591469    59419291 (-172178)  - 40.731 40.689 
( -0.042)


  59589940    59428022 (-161917)  - 40.438 40.322 
( -0.116)

instr delta =  -161917    -0.2717%
time  delta =   -0.116 ms -0.2859%

Tests: hs-tier1-4.
Due to zip library not loaded at default, I removed 'zip' from pmap 
list in test case: *test/hotspot/jtreg/serviceability/sa/ClhsdbPmap.java


**
* Thanks
Yumin

Re: (S) 8237750: Load libzip.so only if necessary

[Dean Long]

Does UseAOT really need libzip?

dl

On 5/1/20 10:24 AM, Yumin Qi wrote:

Hi, please review:
bug 8237750: https://bugs.openjdk.java.net/browse/JDK-8237750
webrev: http://cr.openjdk.java.net/~minqi/8237750/webrev-01/
Summary:
  zip library is loaded unconditionally at start up but it is only 
needed when

  1) bootclasspath contains zip or
  2) UseAOT enabled or
  3) VerifySharedArchive turned on or
  4) CDS archives custom loaded classes
   If none of above in java application, it is not necessary to have 
zip library loaded.


  Solution by loading zip library on demand when needed.

Performance for java -Xint version:

Results of " perf stat -r 50 bin/java -Xshare:on 
-XX:SharedArchiveFile=jdk2.jsa -Xint -version "
   1: 59611556    59450206 (-161350)  - 39.799 40.274 
(  0.475)    ++
   2: 59602708    59425234 (-177474)  - 40.591 41.183 
(  0.592)    ++
   3: 59579718    59441272 (-138446)    40.777 40.471 
( -0.306)  -
   4: 59584882    59410155 (-174727)  - 40.824 40.233 
( -0.591)  --
   5: 59590998    59447252 (-143746)    40.400 40.493 
(  0.093)
   6: 59589523    59441934 (-147589)    40.475 40.064 
( -0.411)  --
   7: 59581820    59413612 (-168208)  - 39.763 40.077 
(  0.314) +
   8: 59593678    59418738 (-174940)  - 40.912 39.724 
( -1.188)  -
   9: 59573058    59412554 (-160504)  - 40.126 40.033 
( -0.093)
  10: 59591469    59419291 (-172178)  - 40.731 40.689 
( -0.042)


  59589940    59428022 (-161917)  - 40.438 40.322 
( -0.116)

instr delta =  -161917    -0.2717%
time  delta =   -0.116 ms -0.2859%

Tests: hs-tier1-4.
Due to zip library not loaded at default, I removed 'zip' from pmap 
list in test case: *test/hotspot/jtreg/serviceability/sa/ClhsdbPmap.java


**
* Thanks
Yumin

Re: RFR (M) Close alignment gaps in InstanceKlass

[Dean Long]

OK, thanks, looks good!

dl

On 4/22/20 7:32 PM, coleen.phillim...@oracle.com wrote:



On 4/22/20 9:00 PM, Dean Long wrote:
It looks like calling the JVMCI getSourceFileName() on an array would 
have accessed random memory because it was expecting an 
InstanceKlass.  Instead of returning null we might want to throw an 
exception like in HotSpotResolvedPrimitiveType.
It was never called, except when I tried to call it on an array in the 
test.  It caused an assert in the hotspot code. How about this? 
Something else in that file throws JVMCIError with a similar message.


    public String getSourceFileName() {
    if (isArray()) {
    throw new JVMCIError("Cannot call getSourceFileName() on 
an array klass type: %s", this);

    }
    return getConstantPool().getSourceFileName();
    }



dl

On 4/22/20 5:39 PM, Dean Long wrote:
Can you compare the result to some string, like "Object.java"?  That 
seems to be what HotSpotResolvedObjectTypeTest.java is doing.
Also, did getSourceFileName() return null for arrays before your 
change?


Fixed:
    public void getSourceFileNameTest() {
    Class c = Object.class;
    ResolvedJavaType type = metaAccess.lookupJavaType(c);
    assertEquals(type.getSourceFileName(), "Object.java");
    }

thanks,
Coleen



dl

On 4/22/20 8:18 AM, coleen.phillim...@oracle.com wrote:


Hi Dean, Thank you for looking at the JVMCI changes and the 
suggestion to add the test.  I did this and found a bug. The new 
test is quite limited because there's no good test to see if a 
source file name can assertNotNull(type.getSourceFileName()), so I 
couldn't iterate through the list of loaded classes like the other 
tests in that file.


http://cr.openjdk.java.net/~coleenp/2020/8238048.02.incr/webrev/index.html 



Thanks,
Coleen


On 4/21/20 9:51 PM, Dean Long wrote:
Hi Coleen.  The JVMCI changes look OK. It looks like there is a 
Graal unittest that covers getSourceFileName, but those tests 
don't always get run. If it's not too much trouble, could you look 
into enabling getSourceFileName() testing in


test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java 



?  It's currently on the "untested" list.

thanks,

dl

On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote:
Summary: moved fields around and some constant fields into 
ConstantPool


This is a simple change except that I moved some constant fields 
from InstanceKlass into the constant pool so they can be shared 
read-only in the CDS archive.  There are associated repercussions 
in SA and JVMCI, so please look at these changes. Also moved 
similarly sized fields together in the class so there's less 
likelihood of introducing gaps in future InstanceKlass changes.


InstanceKlass is reduced from 544 to 520 bytes in a simple Hello 
World class.


open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev

bug link https://bugs.openjdk.java.net/browse/JDK-8238048

Tested with tier1-6.

Thanks,
Coleen

Re: RFR (M) Close alignment gaps in InstanceKlass

[Dean Long]

It looks like calling the JVMCI getSourceFileName() on an array would 
have accessed random memory because it was expecting an InstanceKlass.  
Instead of returning null we might want to throw an exception like in 
HotSpotResolvedPrimitiveType.


dl

On 4/22/20 5:39 PM, Dean Long wrote:
Can you compare the result to some string, like "Object.java"?  That 
seems to be what HotSpotResolvedObjectTypeTest.java is doing.

Also, did getSourceFileName() return null for arrays before your change?

dl

On 4/22/20 8:18 AM, coleen.phillim...@oracle.com wrote:


Hi Dean, Thank you for looking at the JVMCI changes and the 
suggestion to add the test.  I did this and found a bug.  The new 
test is quite limited because there's no good test to see if a source 
file name can assertNotNull(type.getSourceFileName()), so I couldn't 
iterate through the list of loaded classes like the other tests in 
that file.


http://cr.openjdk.java.net/~coleenp/2020/8238048.02.incr/webrev/index.html 



Thanks,
Coleen


On 4/21/20 9:51 PM, Dean Long wrote:
Hi Coleen.  The JVMCI changes look OK. It looks like there is a 
Graal unittest that covers getSourceFileName, but those tests don't 
always get run.  If it's not too much trouble, could you look into 
enabling getSourceFileName() testing in


test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java 



?  It's currently on the "untested" list.

thanks,

dl

On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote:
Summary: moved fields around and some constant fields into 
ConstantPool


This is a simple change except that I moved some constant fields 
from InstanceKlass into the constant pool so they can be shared 
read-only in the CDS archive.  There are associated repercussions 
in SA and JVMCI, so please look at these changes. Also moved 
similarly sized fields together in the class so there's less 
likelihood of introducing gaps in future InstanceKlass changes.


InstanceKlass is reduced from 544 to 520 bytes in a simple Hello 
World class.


open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev

bug link https://bugs.openjdk.java.net/browse/JDK-8238048

Tested with tier1-6.

Thanks,
Coleen

Re: RFR (M) Close alignment gaps in InstanceKlass

[Dean Long]

Can you compare the result to some string, like "Object.java"?  That 
seems to be what HotSpotResolvedObjectTypeTest.java is doing.

Also, did getSourceFileName() return null for arrays before your change?

dl

On 4/22/20 8:18 AM, coleen.phillim...@oracle.com wrote:


Hi Dean, Thank you for looking at the JVMCI changes and the suggestion 
to add the test.  I did this and found a bug.  The new test is quite 
limited because there's no good test to see if a source file name can 
assertNotNull(type.getSourceFileName()), so I couldn't iterate through 
the list of loaded classes like the other tests in that file.


http://cr.openjdk.java.net/~coleenp/2020/8238048.02.incr/webrev/index.html 



Thanks,
Coleen


On 4/21/20 9:51 PM, Dean Long wrote:
Hi Coleen.  The JVMCI changes look OK.  It looks like there is a 
Graal unittest that covers getSourceFileName, but those tests don't 
always get run.  If it's not too much trouble, could you look into 
enabling getSourceFileName() testing in


test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java 



?  It's currently on the "untested" list.

thanks,

dl

On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote:

Summary: moved fields around and some constant fields into ConstantPool

This is a simple change except that I moved some constant fields 
from InstanceKlass into the constant pool so they can be shared 
read-only in the CDS archive.  There are associated repercussions in 
SA and JVMCI, so please look at these changes. Also moved similarly 
sized fields together in the class so there's less likelihood of 
introducing gaps in future InstanceKlass changes.


InstanceKlass is reduced from 544 to 520 bytes in a simple Hello 
World class.


open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev

bug link https://bugs.openjdk.java.net/browse/JDK-8238048

Tested with tier1-6.

Thanks,
Coleen

Re: RFR (M) Close alignment gaps in InstanceKlass

[Dean Long]

Hi Coleen.  The JVMCI changes look OK.  It looks like there is a Graal 
unittest that covers getSourceFileName, but those tests don't always get 
run.  If it's not too much trouble, could you look into enabling 
getSourceFileName() testing in


test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java

?  It's currently on the "untested" list.

thanks,

dl

On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote:

Summary: moved fields around and some constant fields into ConstantPool

This is a simple change except that I moved some constant fields from 
InstanceKlass into the constant pool so they can be shared read-only 
in the CDS archive.  There are associated repercussions in SA and 
JVMCI, so please look at these changes.  Also moved similarly sized 
fields together in the class so there's less likelihood of introducing 
gaps in future InstanceKlass changes.


InstanceKlass is reduced from 544 to 520 bytes in a simple Hello World 
class.


open webrev at http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8238048

Tested with tier1-6.

Thanks,
Coleen

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Dean Long]

I looked at the AOT, C2, and JVMCI changes and I didn't find any issues.

dl

On 3/26/20 4:57 PM, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The main 
changes are in core-libs and hotspot runtime area. Small changes are 
made in javac, VM compiler (intrinsification of Class::isHiddenClass), 
JFR, JDI, and jcmd.  CSR [1]has been reviewed and is in the finalized 
state (see specdiff and javadoc below for reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03

Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not registered 
in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection.  setAccessible(true) can 
still be called on reflected objects representing final fields in a 
hidden class and its access check will be suppressed but only have 
read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

   can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden class
   regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

   and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
   that holds the classes strongly referenced by its defining loader.  
There

   can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access 
control

   check no longer throws LinkageError but instead it will throw IAE with
   a clear message if a class fails to resolve/validate the nest host 
declared

   in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
   and generate a bridge method to desuger a method reference to a 
protected

   method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, and 
LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to 
hidden class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError and 
intends

to have the newly created class linked.  However, the implementation in 14
does not link the class.  A separate CSR [2] proposes to update the
implementation to match the spec.  This patch fixes the implementation.

The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3].  This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden 
classes.


javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/

JVMS 5.4.4 change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf

CSR:
https://bugs.openjdk.java.net/browse/JDK-8238359

Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8238359
[2] https://bugs.openjdk.java.net/browse/JDK-8240338
[3] https://bugs.openjdk.java.net/browse/JDK-8230502

Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

[Dean Long]

You might want to have some runtime/GC folks look at the handshake changes.

dl

On 2/6/20 4:39 AM, Reingruber, Richard wrote:

Hi,

could I please get reviews for this small enhancement:

Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/
Bug:https://bugs.openjdk.java.net/browse/JDK-8238585

The change avoids making all compiled methods on stack not_entrant when 
switching a java thread to
interpreter only execution for jvmti purposes. It is sufficient to deoptimize 
the compiled frames on stack.

Additionally a handshake is used instead of a vm operation to walk the stack 
and do the deoptimizations.

Testing: JCK and JTREG tests, also in Xcomp mode with fastdebug and release 
builds on all platforms.

Thanks, Richard.

See also my question if anyone knows a reason for making the compiled methods 
not_entrant:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030339.html

Re: RFR: JDK-8215196: [Graal] vmTestbase/nsk/jvmti/PopFrame/popframe003/TestDescription.java fails with "changes for the arguments of the popped frame's method, did not remain current argument values"

[Dean Long]

This might be a dumb question, but how is PopFrame used in practice?  
Re-invoking the method, especially with modified argument values seems 
dangerous.


dl

Re: RFR 8226690: SIGSEGV in MetadataOnStackClosure::do_metadata

[dean . long]

Looks good.

dl

On 9/25/19 6:29 PM, coleen.phillim...@oracle.com wrote:



On 9/25/19 9:21 PM, coleen.phillim...@oracle.com wrote:


I see.  I dumped the redefinition count in the replay data because I 
saw the other fields were dumped there.  Would they also not affect 
the generated code?


I can remove these changes.


open webrev at http://cr.openjdk.java.net/~coleenp/2019/8226690.02/webrev

Rebuilt and retested with the ciReplay tests.  Thank you for looking 
at the change.

Coleen


Thanks,
Coleen

On 9/25/19 6:18 PM, dean.l...@oracle.com wrote:
Saving and restoring redefinition_count for compiler replay doesn't 
make sense to me.  It won't affect the generated code, and we 
probably shouldn't even be installing/registering replay nmethods. I 
would remove the ciEnv::dump_replay_data_unsafe() and 
process_JvmtiExport() changes.


dl

On 9/25/19 7:33 AM, coleen.phillim...@oracle.com wrote:
Summary: Dont create nmethod if classes have been redefined since 
compilation start.


The bug was caused by a new nmethod created with an old Method in 
the metadata section.  Added verification (which hit on windows) 
and NSV in the other place where the method can be replaced in the 
nmethod metadata section.


There are some jvmci changes (to vmStructs_jvmci.cpp) that might be 
needed also in the graal compiler.


Tested with tier1-6 and failing test 100 times.

open webrev at 
http://cr.openjdk.java.net/~coleenp/2019/8226690.01/webrev

bug link https://bugs.openjdk.java.net/browse/JDK-8226690

Thanks,
Coleen

Re: RFR 8226690: SIGSEGV in MetadataOnStackClosure::do_metadata

[dean . long]

On 9/25/19 6:21 PM, coleen.phillim...@oracle.com wrote:


I see.  I dumped the redefinition count in the replay data because I 
saw the other fields were dumped there.  Would they also not affect 
the generated code?




I know some like _jvmti_can_access_local_variables can affect the 
generated code.  See


https://github.com/openjdk/jdk/blob/c83c8515ebb4e49fb63c9b896581c9f056268aa0/src/hotspot/share/ci/ciEnv.hpp#L344

dl

I can remove these changes.

Thanks,
Coleen

On 9/25/19 6:18 PM, dean.l...@oracle.com wrote:
Saving and restoring redefinition_count for compiler replay doesn't 
make sense to me.  It won't affect the generated code, and we 
probably shouldn't even be installing/registering replay nmethods. I 
would remove the ciEnv::dump_replay_data_unsafe() and 
process_JvmtiExport() changes.


dl

On 9/25/19 7:33 AM, coleen.phillim...@oracle.com wrote:
Summary: Dont create nmethod if classes have been redefined since 
compilation start.


The bug was caused by a new nmethod created with an old Method in 
the metadata section.  Added verification (which hit on windows) and 
NSV in the other place where the method can be replaced in the 
nmethod metadata section.


There are some jvmci changes (to vmStructs_jvmci.cpp) that might be 
needed also in the graal compiler.


Tested with tier1-6 and failing test 100 times.

open webrev at 
http://cr.openjdk.java.net/~coleenp/2019/8226690.01/webrev

bug link https://bugs.openjdk.java.net/browse/JDK-8226690

Thanks,
Coleen

Re: RFR 8226690: SIGSEGV in MetadataOnStackClosure::do_metadata

[dean . long]

Saving and restoring redefinition_count for compiler replay doesn't make 
sense to me.  It won't affect the generated code, and we probably 
shouldn't even be installing/registering replay nmethods. I would remove 
the ciEnv::dump_replay_data_unsafe() and process_JvmtiExport() changes.


dl

On 9/25/19 7:33 AM, coleen.phillim...@oracle.com wrote:
Summary: Dont create nmethod if classes have been redefined since 
compilation start.


The bug was caused by a new nmethod created with an old Method in the 
metadata section.  Added verification (which hit on windows) and NSV 
in the other place where the method can be replaced in the nmethod 
metadata section.


There are some jvmci changes (to vmStructs_jvmci.cpp) that might be 
needed also in the graal compiler.


Tested with tier1-6 and failing test 100 times.

open webrev at http://cr.openjdk.java.net/~coleenp/2019/8226690.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8226690

Thanks,
Coleen

Re: 8195600: [Graal] jdi tests timeouts with Graal because debuggee vm is not resumed

[dean . long]

I don't have a strong opinion either way.  It's too bad we don't have 
resource management features that would allow setting a memory limit on 
the test or app while allowing the rest of the JVM to use memory 
unrestricted.  That might solve a lot of these OOM problems. Even after 
we move to libgraal, that still doesn't guarantee that reaching an OOM 
state is a recoverable state for the JVM.


Is there a way to rewrite these tests to cover what they need to cover, 
without consuming all of memory (maybe a class unloading WhiteBox API)?  
Or maybe some kind of hint, like a command-line flag, that says the test 
will consume all of memory.  That way any critical services like the 
JVMCI compiler could use that hint to eagerly initialize before the test 
starts.


dl

On 8/27/19 2:08 PM, Daniil Titov wrote:

Hi Dean and Chris,

Just wanted to check with you would it be OK now to add this issue to
Graal-specific problem list, as Dean suggested in one of  the previous emails,
while the proposal about introducing new options for @requires is being 
discussed?

-Thanks!
--Daniil
  



On 8/9/19, 3:37 PM, "hotspot-compiler-dev-boun...@openjdk.java.net on behalf of 
dean.l...@oracle.com"  wrote:

 Good question  When we have libgraal, there will still be an option (at
 least for debugging) to turn it off and use Graal the same way we do
 now, so it seems like the @requires would need to take that into account
 once we have libgraal.  Maybe we will need a new "vm.libgraal.enabled"
 or make "vm.graal.enabled" be false for libgraal?
 
 It does seem a little backwards to require tests to know about the OOM

 handling details of different JVM features.  Instead, how about if we
 let the test assert that it requires "vm.no-background-oom" or whatever,
 and let the JVM decide if it supports it.
 
 CC'ing hotspot-compiler-dev.
 
 dl
 
 On 8/8/19 7:42 PM, Chris Plummer wrote:

 > Actually looking at JDK-8207267 a little closer, it looks like it's
 > job is to re-enable tests that have been disabled with @requires
 > !vm.graal.enabled, so it looks like we have two different approaches
 > going in here. Which is preferred? If the preference is to problem
 > list, do we want to undo JDK-8207261 (except use JDK-8196611 as the CR).
 >
 > Chris
 >
 > On 8/8/19 5:08 PM, Chris Plummer wrote:
 >> That sounds like a better approach to me.
 >>
 >> thanks,
 >>
 >> Chris
 >>
 >> On 8/8/19 4:33 PM, dean.l...@oracle.com wrote:
 >>> This is the kind of failure that is expected to go away with
 >>> libgraal. You can add the tests to the Graal-specific problem list
 >>> (see JDK-8196611) and they should be re-enabled with libgraal (see
 >>> JDK-JDK-8207267).
 >>>
 >>> dl
 >>>
 >>> On 8/8/19 10:21 AM, Chris Plummer wrote:
  Hi Daniil,
 
  My only objection is at some point it seems we need to be able to
  run these tests with graal (and other tests that have been disabled
  due to graal) because graal might be the only compiler, and we'll
  lose test coverage without these tests. Currently we have 260 jtreg
  tests disabled due to graal. I'm not sure to what extent they are
  waiting on graal fixes or otherwise have a bug filed to eventually
  fix them. Would be nice if we had a process in place to make sure
  these issues are eventually addressed. That fact that tests that
  exhaust memory in general seem to be incompatible with graal would
  to be the bigger issue that needs to be addressed.
 
  thanks,
 
  Chris
 
  On 8/7/19 3:38 PM, Daniil Titov wrote:
 > Please review the change that fixes the failing tests when running
 > with Graal. The issue originally
 > included several vmTestbase/nsk/jdi tests but only 2 of them still
 > fail:
 > -
 > 
vmTestbase/nsk/jdi/VirtualMachine/instanceCounts/instancecounts003/instancecounts003.java
 > -
 > 
vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java
 >
 > The problem with these two tests is that they consume all memory
 > to force the class unloading that
 > results in the exception during JVMCI compiler initialization and
 > the test failure.
 >   The fix filters these tests out to not run with Graal compiler.
 >
 > Webrev: http://cr.openjdk.java.net/~dtitov/8195600/webrev.01/
 > Bug: https://bugs.openjdk.java.net/browse/JDK-8195600
 >
 > Thanks,
 > Daniil
 >
 >
 
 >>>
 >>
 >>
 >
 >
 
 
 

Re: RFR: 8195600: [Graal] jdi tests timeouts with Graal because debuggee vm is not resumed

[dean . long]

Good question  When we have libgraal, there will still be an option (at 
least for debugging) to turn it off and use Graal the same way we do 
now, so it seems like the @requires would need to take that into account 
once we have libgraal.  Maybe we will need a new "vm.libgraal.enabled" 
or make "vm.graal.enabled" be false for libgraal?


It does seem a little backwards to require tests to know about the OOM 
handling details of different JVM features.  Instead, how about if we 
let the test assert that it requires "vm.no-background-oom" or whatever, 
and let the JVM decide if it supports it.


CC'ing hotspot-compiler-dev.

dl

On 8/8/19 7:42 PM, Chris Plummer wrote:
Actually looking at JDK-8207267 a little closer, it looks like it's 
job is to re-enable tests that have been disabled with @requires 
!vm.graal.enabled, so it looks like we have two different approaches 
going in here. Which is preferred? If the preference is to problem 
list, do we want to undo JDK-8207261 (except use JDK-8196611 as the CR).


Chris

On 8/8/19 5:08 PM, Chris Plummer wrote:

That sounds like a better approach to me.

thanks,

Chris

On 8/8/19 4:33 PM, dean.l...@oracle.com wrote:
This is the kind of failure that is expected to go away with 
libgraal. You can add the tests to the Graal-specific problem list 
(see JDK-8196611) and they should be re-enabled with libgraal (see 
JDK-JDK-8207267).


dl

On 8/8/19 10:21 AM, Chris Plummer wrote:

Hi Daniil,

My only objection is at some point it seems we need to be able to 
run these tests with graal (and other tests that have been disabled 
due to graal) because graal might be the only compiler, and we'll 
lose test coverage without these tests. Currently we have 260 jtreg 
tests disabled due to graal. I'm not sure to what extent they are 
waiting on graal fixes or otherwise have a bug filed to eventually 
fix them. Would be nice if we had a process in place to make sure 
these issues are eventually addressed. That fact that tests that 
exhaust memory in general seem to be incompatible with graal would 
to be the bigger issue that needs to be addressed.


thanks,

Chris

On 8/7/19 3:38 PM, Daniil Titov wrote:
Please review the change that fixes the failing tests when running 
with Graal. The issue originally
included several vmTestbase/nsk/jdi tests but only 2 of them still 
fail:
- 
vmTestbase/nsk/jdi/VirtualMachine/instanceCounts/instancecounts003/instancecounts003.java
- 
vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java


The problem with these two tests is that they consume all memory 
to force the class unloading that
results in the exception during JVMCI compiler initialization and 
the test failure.

  The fix filters these tests out to not run with Graal compiler.

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

Thanks,
Daniil

Re: RFR: 8195600: [Graal] jdi tests timeouts with Graal because debuggee vm is not resumed

[dean . long]

This is the kind of failure that is expected to go away with libgraal.  
You can add the tests to the Graal-specific problem list (see 
JDK-8196611) and they should be re-enabled with libgraal (see 
JDK-JDK-8207267).


dl

On 8/8/19 10:21 AM, Chris Plummer wrote:

Hi Daniil,

My only objection is at some point it seems we need to be able to run 
these tests with graal (and other tests that have been disabled due to 
graal) because graal might be the only compiler, and we'll lose test 
coverage without these tests. Currently we have 260 jtreg tests 
disabled due to graal. I'm not sure to what extent they are waiting on 
graal fixes or otherwise have a bug filed to eventually fix them. 
Would be nice if we had a process in place to make sure these issues 
are eventually addressed. That fact that tests that exhaust memory in 
general seem to be incompatible with graal would to be the bigger 
issue that needs to be addressed.


thanks,

Chris

On 8/7/19 3:38 PM, Daniil Titov wrote:
Please review the change that fixes the failing tests when running 
with Graal. The issue originally

included several vmTestbase/nsk/jdi tests but only 2 of them still fail:
- 
vmTestbase/nsk/jdi/VirtualMachine/instanceCounts/instancecounts003/instancecounts003.java
- 
vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java


The problem with these two tests is that they consume all memory to 
force the class unloading that
results in the exception during JVMCI compiler initialization and the 
test failure.

  The fix filters these tests out to not run with Graal compiler.

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

Thanks,
Daniil

Re: RFR (S) 8225437: JvmtiExport::gc_epilogue is unnecessary

[dean . long]

I don't see it removed from the .hpp files.

dl

On 6/26/19 8:02 AM, coleen.phillim...@oracle.com wrote:

Summary: Remove jvmtiExport::gc_epilogue after full GCs

See bug for more information.

open webrev at http://cr.openjdk.java.net/~coleenp/2019/8225437.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8225437

Tested with hs-tier1-3.

Thanks,
Coleen

Re: RFR: 8222821: com/sun/jdi/ExceptionEvents.java failed

[dean . long]

Looks good.

dl

On 4/25/19 6:33 PM, Daniil Titov wrote:

Please review the change that fixes an intermittent failure of the test when 
running with Graal on.

The test creates exception requests for different kinds of exceptions and 
errors, starts the debuggee that throws an exception, and listens for exception 
events. If the number of received exception events is not equal to 1 the test 
fails. For the case when the exception request is created for java.lang.Error 
class  the test intermittently fails if Graal is on. It happens because, 
sometimes, in addition to StackOverflowError thrown by the test itself, 
jdk.vm.ci.common.JVMCIError is thrown from method getField() in class 
jdk.vm.ci.hotspot .HotSpotVMConfigAccess at line 252 
(src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotVMConfigAccess.java)

240 private VMField getField(String name, String cppType, boolean 
required) {
241 VMField entry = store.vmFields.get(name);
242 if (entry == null) {
243 if (!required) {
244 return null;
245 }
246 store.printConfig();
247 throw new JVMCIError("expected VM field not found in " + store + 
": " + name);
248 }
249 
250 // Make sure the native type is still the type we expect.
251 if (cppType != null && !cppType.equals(entry.type)) {
252 throw new JVMCIError("expected type " + cppType + " but VM field " + 
name + " is of type " + entry.type);
253 }
254 return entry;
255 }

that in one case is caught at line 412 in 
src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java

404 public final int pendingFailedSpeculationOffset;
405 {
406 String name = "JavaThread::_pending_failed_speculation";
407 int offset = -1;
408 try {
409 offset = getFieldOffset(name, Integer.class, "jlong");
410 } catch (JVMCIError e) {
411 try {
412 offset = getFieldOffset(name, Integer.class, "long");
413 } catch (JVMCIError e2) {
414 }
415 }
416 if (offset == -1) {
417 throw new JVMCIError("cannot get offset of field " + name + " 
with type long or jlong");
418 }
419 pendingFailedSpeculationOffset = offset;
420 }

and in other case at line 229 in the same class 
(src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java)

221 public final int classMirrorOffset;
222 {
223 String name = "Klass::_java_mirror";
224 int offset = -1;
225 boolean isHandle = false;
226 try {
227 offset = getFieldOffset(name, Integer.class, "oop");
228 } catch (JVMCIError e) {
229 
230 }
231 if (offset == -1) {
232 try {
233 offset = getFieldOffset(name, Integer.class, "jobject");
234 isHandle = true;
235 } catch (JVMCIError e) {
236 try {
237 // JDK-8186777
238 offset = getFieldOffset(name, Integer.class, 
"OopHandle");
239 isHandle = true;
240 } catch (JVMCIError e2) {
241 }
242 }
243 }
244 if (offset == -1) {
245 throw new JVMCIError("cannot get offset of field " + name + " 
with type oop, jobject or OopHandle");
246 }
247 classMirrorOffset = offset;
248 classMirrorIsHandle = isHandle;
249 }

That results in the number of received exception events exceeds 1 and the test 
fails.

To ignore these unexpected events the fix adds "jdk.vm.ci.hotspot.*"  class 
exclusion filter when it creates an exception request.

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

Thanks!
-Daniil

Re: RFR: JDK-8218166: [Graal] com/sun/jdi/SimulResumerTest.java failure

[dean . long]

I don't know how to interpret "ignore checks if thread was collected", 
so it doesn't add much value for me.

How about something like "ignore ownedMonitors() failure"?

dl

On 3/13/19 8:54 AM, Gary Adams wrote:

One last set of diffs ...
  - added comments on the ignored exceptions
  - commented out excessive diagnostic print out
 (this will remove the jtreg truncated output)

Ok to use dan, dean and jc as reveiwers?

diff --git a/test/jdk/com/sun/jdi/SimulResumerTest.java 
b/test/jdk/com/sun/jdi/SimulResumerTest.java

--- a/test/jdk/com/sun/jdi/SimulResumerTest.java
+++ b/test/jdk/com/sun/jdi/SimulResumerTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2015, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2008, 2019, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -210,7 +210,9 @@
 }

 } catch (IncompatibleThreadStateException ee) {
-    // ignore
+    // ignore checks if thread was not suspended
+    } catch (ObjectCollectedException ee) {
+    // ignore checks if thread was collected
 } catch (VMDisconnectedException ee) {
 // This is how we stop.  The debuggee runs to completion
 // and we get this exception.
@@ -249,7 +251,7 @@
 public void run() {
 while (true) {
 iters++;
-    System.out.println("bkpts = " + bkpts + ", 
iters = " + iters);
+    // System.out.println("bkpts = " + bkpts + ", 
iters = " + iters);

 try {
 Thread.sleep(waitTime);
 check(debuggeeThread1);



On 3/7/19, 8:19 AM, Gary Adams wrote:

While trying to reproduce the timeout reported in
  JDK-8000669: com/sun/jdi/SimulResumerTest.java times out

I was unable to reproduce the timeout failure, but I did occasionally
see the ObjectCollectedException. The output from the test is very 
verbose
and may be the source of the occasional timeout. I'd like to close 
JDK-8000669
as cannot reproduce and if it shows up again look into limiting the 
amount

of non-essential output from the test.

This is a racy test to begin with and it already is ignoring exceptions
due to unexpected thread states. Adding the ignore for 
ObjectCollectedException

allows the test to complete without errors.

The graal label was recently removed. We should also remove it from 
the summary.


Proposed changeset:


diff --git a/test/jdk/com/sun/jdi/SimulResumerTest.java 
b/test/jdk/com/sun/jdi/SimulResumerTest.java

--- a/test/jdk/com/sun/jdi/SimulResumerTest.java
+++ b/test/jdk/com/sun/jdi/SimulResumerTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2015, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2008, 2019, Oracle and/or its affiliates. All 
rights reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -211,6 +211,8 @@

 } catch (IncompatibleThreadStateException ee) {
 // ignore
+    } catch (ObjectCollectedException ee) {
+    // ignore
 } catch (VMDisconnectedException ee) {
 // This is how we stop.  The debuggee runs to 
completion

 // and we get this exception.

Re: RFR: 8220244: vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t003 hasn't been un-problemlisted

[dean . long]

Looks good and trivial.

dl

On 3/12/19 4:46 PM, Daniil Titov wrote:

Please review a trivial change that removes test 
vmTestbase/nsk/jvmti/scenarios/sampling/SP06/sp06t003, recently fixed in 
8218167, from ProblemList-graal.txt

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

Thanks!
--Daniil

Re: RFR: JDK-8218166: [Graal] com/sun/jdi/SimulResumerTest.java failure

[dean . long]

Looks good to me.

dl


On 3/12/19 11:07 AM, Gary Adams wrote:

Still need 2 more reviewers ...

On 3/7/19, 9:45 AM, Daniel D. Daugherty wrote:

Gary,

Since the 'graal' label was recently removed, I also removed "[Graal]"
from the bug synopsis. Please make sure you update your commit mesg.


On 3/7/19 8:19 AM, Gary Adams wrote:

While trying to reproduce the timeout reported in
  JDK-8000669: com/sun/jdi/SimulResumerTest.java times out

I was unable to reproduce the timeout failure, but I did occasionally
see the ObjectCollectedException. The output from the test is very 
verbose
and may be the source of the occasional timeout. I'd like to close 
JDK-8000669
as cannot reproduce and if it shows up again look into limiting the 
amount

of non-essential output from the test.

This is a racy test to begin with and it already is ignoring exceptions
due to unexpected thread states. Adding the ignore for 
ObjectCollectedException

allows the test to complete without errors.

The graal label was recently removed. We should also remove it from 
the summary.


Proposed changeset:


diff --git a/test/jdk/com/sun/jdi/SimulResumerTest.java 
b/test/jdk/com/sun/jdi/SimulResumerTest.java

--- a/test/jdk/com/sun/jdi/SimulResumerTest.java
+++ b/test/jdk/com/sun/jdi/SimulResumerTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2015, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2008, 2019, Oracle and/or its affiliates. All 
rights reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or 
modify it

@@ -211,6 +211,8 @@

 } catch (IncompatibleThreadStateException ee) {
 // ignore
+    } catch (ObjectCollectedException ee) {
+    // ignore
 } catch (VMDisconnectedException ee) {
 // This is how we stop.  The debuggee runs to 
completion

 // and we get this exception.


There should be some sort of comment explaining why it is okay to ignore
the ObjectCollectedException. When the IncompatibleThreadStateException
was ignored, there should have been a comment added for that also.

Dan

Re: 8218812: vmTestbase/nsk/jvmti/GetAllThreads/allthr001/TestDescription.java failed

[dean . long]

This is consistent with how other system threads are handled, so it 
looks good to me.


dl

On 3/11/19 4:32 PM, Daniil Titov wrote:

Hi Dean, JC and Daniel,

Thank you for reviewing this change. Based on the overall output it seems as 
the common consensus is that the broader discussions is required to decide what 
would be a proper way to optionally hide/mute JVMCI compiler threads (and 
probably some other “system” Java threads) from JVMTI clients.  Thus, my 
suggestion is to move this discussion in a new enhancement [3] and limit the 
current issue just to the fixing the test itself.

Please review a new version of the webrev that adds  JVMCI compiler and 
"HotSpotGraalManagement Bean Registration" threads to the list of the threads 
the tests must ignore.
  


Reference:
--
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8218812/webrev.02
[2] Bug: https://bugs.openjdk.java.net/browse/JDK-8218812
[3] Enhancement: https://bugs.openjdk.java.net/browse/JDK-8220468

Thanks,
Daniil

From: 
Organization: Oracle Corporation
Date: Thursday, March 7, 2019 at 12:19 PM
To: Daniil Titov , OpenJDK Serviceability 

Subject: Re: RFR: 8218812: 
vmTestbase/nsk/jvmti/GetAllThreads/allthr001/TestDescription.java failed

There are other places where is_hidden_from_external_view() is used.  Should is_hidden_from_external_view() also check the new 
capability?  If so, then you can simplify your changes.  I'm not sure the new capability is the best choice, however.  There is 
still no way to control whether compiler threads can post events, hit breakpoints, single-step, etc.  And "compiler 
thread" might be too specific.  There might be other types of "system thread" that we want to ignore.  Since this 
is a JVMTI spec change, I think it deserves more discussion.  For example, an alternative would be a way to set "can 
debug"/"visible"/"can post events"/etc flags on individual threads.

dl
On 3/7/19 9:54 AM, Daniil Titov wrote:
Please review a change that fixes this test.
  
The problem here is that the test checks the number of threads and with Graal on additional threads the test doesn't expect are started and cause the test fail.
  
The fix introduces a new capability " can_show_compiler_threads" that affects whether Java compiler threads are retuned with JVMTI GetAllThreads call. By default this capability is off. The fix also adds " HotSpotGraalManagement Bean Registration" thread to the list of the threads the tests must ignore.
  
Webrev: http://cr.openjdk.java.net/~dtitov/8218812/webrev.01

Bug: https://bugs.openjdk.java.net/browse/JDK-8218812
  
Mach5 tier1, tier2 and tier3 tests successfully passed with this change.
  
Thanks!

-Daniil
  

Re: RFR: 8218812: vmTestbase/nsk/jvmti/GetAllThreads/allthr001/TestDescription.java failed

[dean . long]

This part seems to change the value for the 
include_jni_attaching_threads arg from true to false:


- ThreadsListEnumerator tle(Thread::current(), true);
+ ThreadsListEnumerator tle(Thread::current(), true, false, 
JvmtiExport::should_show_compiler_threads()); dl

Re: RFR: 8218812: vmTestbase/nsk/jvmti/GetAllThreads/allthr001/TestDescription.java failed

[dean . long]

There are other places where is_hidden_from_external_view() is used.  
Should is_hidden_from_external_view() also check the new capability?  If 
so, then you can simplify your changes.  I'm not sure the new capability 
is the best choice, however.  There is still no way to control whether 
compiler threads can post events, hit breakpoints, single-step, etc.  
And "compiler thread" might be too specific.  There might be other types 
of "system thread" that we want to ignore.  Since this is a JVMTI spec 
change, I think it deserves more discussion.  For example, an 
alternative would be a way to set "can debug"/"visible"/"can post 
events"/etc flags on individual threads.


dl

On 3/7/19 9:54 AM, Daniil Titov wrote:


Please review a change that fixes this test.

The problem here is that the test checks the number of threads and 
with Graal on additional threads the test doesn't expect are started 
and cause the test fail.


The fix introduces a new capability " can_show_compiler_threads" that 
affects whether Java compiler threads are retuned with JVMTI 
GetAllThreads call. By default this capability is off. The fix also 
adds " HotSpotGraalManagement Bean Registration" thread to the list of 
the threads the tests must ignore.


Webrev: http://cr.openjdk.java.net/~dtitov/8218812/webrev.01

Bug: https://bugs.openjdk.java.net/browse/JDK-8218812

Mach5 tier1, tier2 and tier3 tests successfully passed with this change.

Thanks!

-Daniil

Re: RFR 8218167: nsk/jvmti/scenarios/sampling/SP02/sp02t003 fails

[dean . long]

Looks good, but what about sp06t003?  Doesn't it have the same problem?  
Are there any other tests using similar logic?


dl

On 3/1/19 8:33 PM, Daniil Titov wrote:

Please review the change that fix intermittent failure for test 
nsk/jvmti/scenarios/sampling/SP02/sp02t003 when running with Graal.

The problem with the test here is that method checkThread() looks for the test method in the top "commonDepth" 
frames where "commonDepth" is a minimum of "frameCount" (returned by jvmti->GetFrameCount) and 
"frameStackSize"( returned by jvmti->GetStackTrace).

If a compilation is triggered between these 2 calls then there are cases when 
"frameCount"  is 2,  "frameStackSize" is 4,  and the frame stack is as the 
following:

[0] adjustCompilationLevel
[1] adjustCompilationLevel
[2] testedMethod
[3] run

In this case the test looks for the test method only in 2 top frames and fails.

The fix ensures that the test iterates over all frames in the frame stack when 
looking for the test method.

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

Thanks!
--Daniil

Re: RFR (M) 8139551: Scalability problem with redefinition - multiple code cache walks

[dean . long]

Could you move this code:

 // Mark dependent AOT nmethods, which are only found via the class 
redefined.

 AOTLoader::mark_evol_dependent_methods(ik);

into this function:

 CodeCache::mark_for_evol_deoptimization(ik)

The rest looks good.

dl

On 2/4/19 7:08 AM, coleen.phillim...@oracle.com wrote:

Summary: Walk code cache and deoptimize once per redefinition.*

*This change touches the AOT and code cache code.  I tried to describe 
it in the comments in the RFE.  Basically, for redefinition, we walk 
the code cache per class redefined in order to find evol_method 
dependencies, then deoptimize if we find them, and then walk the code 
cache again to mark the deoptimized nmethods as not_entrant.


I replaced this with only marking any nmethods for the class's methods 
to deoptimize (following InstanceKlass::_methods), also marking aot 
methods dependent on the class, and then doing the code cache walk to 
follow the evol_method dependencies at the end of redefinition for all 
the classes.   The new code uses the Method::is_old() flag rather than 
comparing each method in the InstanceKlass.  Then deoptimization and 
marking not_entrant is done once at the end of the redefinition as well.


Tested with tier1-6, all the redefinition tests locally:

make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jvmti >
make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jdi >
make test TEST=open/test/hotspot/jtreg/runtime/RedefineTests 
>

make test TEST=open/test/jdk/java/lang/instrument >

new test, and JMH test (see CR for results).

Thanks,
Coleen

Re: RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

[dean . long]

On 1/30/19 8:59 PM, serguei.spit...@oracle.com wrote:

So, the fix needs to be more like this:
+ // Workaround for 8195635:
+ // disable pop_frame and force_early_return capabilities with Graal
+ #if INCLUDE_JVMCI
+ if (!(EnableJVMCI && UseJVMCICompiler)) {
jc.can_pop_frame = 1;
jc.can_force_early_return = 1;
+ } + #endif Not sure, if the check for EnableJVMCI can be removed above.


We still need it to work when INCLUDE_JVMCI is not defined.
How about

JVMCI_ONLY(if (UseJVMCICompiler)) {
...
}

or

if (JVMCI_ONLY(UseJVMCICompiler) NOT_JVMCI(true)) {
...
}

dl

Re: 12 RFR(M) 8195635: [Graal] nsk/jvmti/unit/ForceEarlyReturn/earlyretbase crashes with assertion "compilation level out of bounds"

[dean . long]

I'm withdrawing this RFR for 12.

dl

On 1/28/19 5:13 PM, dean.l...@oracle.com wrote:

http://cr.openjdk.java.net/~dlong/8195635/webrev.5/
https://bugs.openjdk.java.net/browse/JDK-8195635

Please see the bug report for all the gory details.  Here's the short 
version:


If we allow any safepoint to be a suspend point, we run into trouble 
with PopFrame and ForceEarlyReturn, which reasonably expect the top 
frame not to change between the suspend and when the 
PopFrame/ForceEarlyReturn is executed.  Normally this is not an issue, 
but certain safepoints cause problems, when we are about to call a new 
Java method.  In particular, if we safepoint and suspend in 
JavaCallWrapper, the top frame will still be the caller, but when we 
execute the PopFrame/ForceEarlyReturn we will be in the callee.


The solution this patch takes is to block suspend around troublesome 
VM code using a new "allow_suspend" thread flag.  This means 
JavaThread::java_suspend can't just ask the VMThread to safepoint and 
be done.  Instead it has wait and allow threads to roll forward to an 
allowed suspend point.


dl

12 RFR(M) 8195635: [Graal] nsk/jvmti/unit/ForceEarlyReturn/earlyretbase crashes with assertion "compilation level out of bounds"

[dean . long]

http://cr.openjdk.java.net/~dlong/8195635/webrev.5/
https://bugs.openjdk.java.net/browse/JDK-8195635

Please see the bug report for all the gory details.  Here's the short 
version:


If we allow any safepoint to be a suspend point, we run into trouble 
with PopFrame and ForceEarlyReturn, which reasonably expect the top 
frame not to change between the suspend and when the 
PopFrame/ForceEarlyReturn is executed.  Normally this is not an issue, 
but certain safepoints cause problems, when we are about to call a new 
Java method.  In particular, if we safepoint and suspend in 
JavaCallWrapper, the top frame will still be the caller, but when we 
execute the PopFrame/ForceEarlyReturn we will be in the callee.


The solution this patch takes is to block suspend around troublesome VM 
code using a new "allow_suspend" thread flag.  This means 
JavaThread::java_suspend can't just ask the VMThread to safepoint and be 
done.  Instead it has wait and allow threads to roll forward to an 
allowed suspend point.


dl

Re: RFR: 8217618: JVM TI SuspendThread doesn't suspend the current thread before returning

[dean . long]

On 1/26/19 7:43 PM, Daniel D. Daugherty wrote:

However, java_suspend_self() is careful and only self-suspends if
is_external_suspend() is still true (and it makes that check while
it owns the SR_lock). The code in java_suspend_self() has to be
careful in both directions. You don't want it to self-suspend
when it has been resumed by a racer and you don't want it to
resume if there was another SuspendThread() call was made and
has returned to it's caller. Check out the comments in
java_suspend_self(); they should help clarify things. 


Thanks for the explanation.  I was assuming that if you call suspend
and resume without synchronization, then all bets are off.  But since
we already have some guarantees in place, it makes sense to
preserve them.

dl

Re: RFR: 8217618: JVM TI SuspendThread doesn't suspend the current thread before returning

[dean . long]

I think there is going to be a race with resume wherever you put the 
self-suspend, because we do it without holding SR_lock.  So you should 
be able to move the self check and self suspend to before the SR_lock.  
This would just be a performance optimization.


dl

On 1/24/19 6:46 PM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8217618
webrev: http://cr.openjdk.java.net/~dholmes/8217618/webrev/

Lots of analysis in the bug report. Bottom line: SuspendThread of the 
current thread was not actually suspending the thread until it hit 
specific thread-state transitions. That meant that SuspendThread would 
actually return and continue executing native code whilst suspended, 
in violation of the specification for it.


The fix is quite simple: in java_suspend() we check for the current 
thread and call java_suspend_self().


Testing:
 - Any test that looked like it referred to thread suspension
  - hotspot/jtreg/vmTestbase/nsk/jvmti/*
  - jdk/
 com/sun/jdi/*
 java/lang/ThreadGroup/Suspend.java
java/lang/management/CompositeData/ThreadInfoCompositeData.java
 java/lang/management/ThreadMXBean/*
 java/nio/channels/SocketChannel/SendUrgentData.java
java/util/logging/LogManager/Configuration/TestConfigurationLock.java

 - Mach 5 tiers 1-3 (in progress)

Two tests were found to be erroneously relying on SuspendThread 
returning whilst suspended:


- vmTestbase/nsk/jvmti/scenarios/sampling/SP05/sp05t003/sp05t003.cpp

The test updated a shared counter after the SuspendThread call, but it 
needed to be updated before the call.


- vmTestbase/nsk/jvmti/scenarios/hotswap/HS202/hs202t002/hs202t002.cpp

The test was using a 0 return value from SuspendThread as an indicator 
that the thread was in the suspended state - but that value can't be 
seen until after SuspendThread returns, which is after the thread is 
resumed. So I ripped out the custom state tracking logic and replaced 
with a simple check of GetThreadState.


Thanks,
David

Re: RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the thread when the top frame executes JVMCI code

[dean . long]

On 11/30/18 7:46 PM, JC Beyler wrote:
Questions because I'm not familiar with JVMCI consequences so not 
really comments on the webrev but so that I know:

  - Is it normaly that you can suspend when you are in a JVMCI frame?


Yes, because it's just Java code, and we allow all Java code to be 
suspended, even Graal and JVMCI code.


will/is there not a better way that we could detect that we are in a 
JVMCI frame?


We could check the threads's _adjusting_comp_level flag for this 
particular case, if we decided that we don't want to be able to debug 
JVMCI Java code.



Is it always safe to suspend a JVMCI frame?


That's a good question.  If it was grabbing any locks, then suspending 
it could cause problems for other threads calling into JVMCI.


Another solution would be to do adjusting_comp_level() in a separate 
thread.  So I think there are at least 3 possible solutions:


1) Allow JVMCI adjusting_comp_level call to be suspended/debugged
2) Don't allow it to be suspended/debugged,
    a) by running in a separate thread, or
    b) don't suspend when _adjusting_comp_level flag is set

We could introduce a concept of "system Java" code, which, just like 
Unix kernel code that is not debuggable without a kernel debugger, would 
not normally be debuggable without setting a special flag.


CCing graal-dev alias.

dl

Re: RFR(XS) 8193577: nsk/jvmti/IterateThroughHeap/filter-tagged fails with Graal in Xcomp mode

[dean . long]

OK your fix looks good.

dl

On 11/28/18 10:57 PM, Igor Veresov wrote:
It would work right now. But I don’t want us fixing it again when 
somebody implements effectively final optimization in Graal.


igor



On Nov 28, 2018, at 10:02 PM, dean.l...@oracle.com 
 wrote:


I like it better.  But do you really need to use JNI to reset the 
values?  If you had


static int POISON = 0x1234;  // not final

class TaggedClass {
  static int field1 = 0xC0DE01 + POISON;

in HeapFilter.java, is the compiler smart enough to treat the value 
as constant until it changes?


dl

On 11/28/18 9:26 PM, Igor Veresov wrote:
Alright, how about the following solution: 
http://cr.openjdk.java.net/~iveresov/8193577/webrev.01/


igor



On Nov 28, 2018, at 4:59 PM, Igor Veresov > wrote:


I don’t want to make it Graal specific. I think I’ll just do field 
assignments in native so that it’s all invisible to the compiler.


igor



On Nov 28, 2018, at 3:25 PM, serguei.spit...@oracle.com 
 wrote:


On 11/28/18 15:16, dean.l...@oracle.com wrote:
It sounds like the test could also fail with C2 if the fields are 
in a virtual object that was eliminated.  I'm OK with your fix, 
but I would feel a little better if we only relaxed the check for 
Graal.  I guess you'd need to use the whitebox api for that.


I was thinking about the same.
Relaxing this just for Graal sounds good.
On the other hand, making the test to know about Graal looks a 
little bit strange. :)


Thanks,
Serguei



dl

On 11/28/18 2:28 PM, Igor Veresov wrote:
Oh, I haven’t understood your idea before pressing reply. Yes, 
we can match the objects by matching their shape, but it’s also 
not an exact solution prone to erroneous matches. Especially 
considering the iteration API does callbacks for the fields out 
of order - it does primitives, strings, arrays, in that order.


There are also ways to make it fail with Graal that are not 
related to constant representation. Graal treats allocations as 
side effect free. So it’s possible to allocate something and 
then deopt to a point before the allocation and redo the 
allocation in the interpreter. In this case there are going to 
be multiple objects on the heap with only one of them being 
reachable.


igor



On Nov 28, 2018, at 2:08 PM, Igor Veresov 
mailto:igor.vere...@oracle.com>> wrote:


I don’t think there is a way to identify an untagged object. 
There is nothing that is passed in the callback to allow that.


igor


On Nov 28, 2018, at 1:32 PM, dean.l...@oracle.com 
 wrote:


I'm guessing Graal creates Constant boxes of the individual 
fields and not of the test objects?  If so, can't we fix the 
matching so that it checks that all fields of an object match? 
I guess that would mean moving the "expected" and "found" 
counts up from fields[] to objects_info[].


dl

On 11/28/18 1:13 PM, Igor Veresov wrote:
When doing heap iteration with JVMTI the way the object 
identity is tracked is by tagging. This particular test 
checks if it  can observe an untagged object. Since there is 
no way to truly track the identity of an untagged object the 
test validates the identity by checking the value of the 
fields of such object. When being compiled with Graal there 
are objects produced (such as Constant boxes) that have field 
values that are equal to the expected values for the fields 
in UntaggedClass. During the subsequent heap iteration these 
objects are reported to the test and are treated as if they 
were instances of UntaggedClass.


The fix is to relax the test requirement and allow (for the 
untagged case) the number of the object found during 
iteration to be more than expected.


Webrev: 
http://cr.openjdk.java.net/~iveresov/8193577/webrev.00/ 


JBS: https://bugs.openjdk.java.net/browse/JDK-8193577

igor

Re: RFR(XS) 8193577: nsk/jvmti/IterateThroughHeap/filter-tagged fails with Graal in Xcomp mode

[dean . long]

I like it better.  But do you really need to use JNI to reset the 
values?  If you had


static int POISON = 0x1234;  // not final

class TaggedClass {
  static int field1 = 0xC0DE01 + POISON;

in HeapFilter.java, is the compiler smart enough to treat the value as 
constant until it changes?


dl

On 11/28/18 9:26 PM, Igor Veresov wrote:
Alright, how about the following solution: 
http://cr.openjdk.java.net/~iveresov/8193577/webrev.01/


igor



On Nov 28, 2018, at 4:59 PM, Igor Veresov > wrote:


I don’t want to make it Graal specific. I think I’ll just do field 
assignments in native so that it’s all invisible to the compiler.


igor



On Nov 28, 2018, at 3:25 PM, serguei.spit...@oracle.com 
 wrote:


On 11/28/18 15:16, dean.l...@oracle.com wrote:
It sounds like the test could also fail with C2 if the fields are 
in a virtual object that was eliminated.  I'm OK with your fix, but 
I would feel a little better if we only relaxed the check for 
Graal.  I guess you'd need to use the whitebox api for that.


I was thinking about the same.
Relaxing this just for Graal sounds good.
On the other hand, making the test to know about Graal looks a 
little bit strange. :)


Thanks,
Serguei



dl

On 11/28/18 2:28 PM, Igor Veresov wrote:
Oh, I haven’t understood your idea before pressing reply. Yes, we 
can match the objects by matching their shape, but it’s also not 
an exact solution prone to erroneous matches. Especially 
considering the iteration API does callbacks for the fields out of 
order - it does primitives, strings, arrays, in that order.


There are also ways to make it fail with Graal that are not 
related to constant representation. Graal treats allocations as 
side effect free. So it’s possible to allocate something and then 
deopt to a point before the allocation and redo the allocation in 
the interpreter. In this case there are going to be multiple 
objects on the heap with only one of them being reachable.


igor



On Nov 28, 2018, at 2:08 PM, Igor Veresov 
mailto:igor.vere...@oracle.com>> wrote:


I don’t think there is a way to identify an untagged object. 
There is nothing that is passed in the callback to allow that.


igor


On Nov 28, 2018, at 1:32 PM, dean.l...@oracle.com 
 wrote:


I'm guessing Graal creates Constant boxes of the individual 
fields and not of the test objects?  If so, can't we fix the 
matching so that it checks that all fields of an object match?  
I guess that would mean moving the "expected" and "found" counts 
up from fields[] to objects_info[].


dl

On 11/28/18 1:13 PM, Igor Veresov wrote:
When doing heap iteration with JVMTI the way the object 
identity is tracked is by tagging. This particular test checks 
if it  can observe an untagged object. Since there is no way to 
truly track the identity of an untagged object the test 
validates the identity by checking the value of the fields of 
such object. When being compiled with Graal there are objects 
produced (such as Constant boxes) that have field values that 
are equal to the expected values for the fields in 
UntaggedClass. During the subsequent heap iteration these 
objects are reported to the test and are treated as if they 
were instances of UntaggedClass.


The fix is to relax the test requirement and allow (for the 
untagged case) the number of the object found during iteration 
to be more than expected.


Webrev: http://cr.openjdk.java.net/~iveresov/8193577/webrev.00/ 


JBS: https://bugs.openjdk.java.net/browse/JDK-8193577

igor

Re: RFR(XS) 8193577: nsk/jvmti/IterateThroughHeap/filter-tagged fails with Graal in Xcomp mode

[dean . long]

It sounds like the test could also fail with C2 if the fields are in a 
virtual object that was eliminated.  I'm OK with your fix, but I would 
feel a little better if we only relaxed the check for Graal. I guess 
you'd need to use the whitebox api for that.


dl

On 11/28/18 2:28 PM, Igor Veresov wrote:
Oh, I haven’t understood your idea before pressing reply. Yes, we can 
match the objects by matching their shape, but it’s also not an exact 
solution prone to erroneous matches. Especially considering the 
iteration API does callbacks for the fields out of order - it does 
primitives, strings, arrays, in that order.


There are also ways to make it fail with Graal that are not related to 
constant representation. Graal treats allocations as side effect free. 
So it’s possible to allocate something and then deopt to a point 
before the allocation and redo the allocation in the interpreter. In 
this case there are going to be multiple objects on the heap with only 
one of them being reachable.


igor



On Nov 28, 2018, at 2:08 PM, Igor Veresov > wrote:


I don’t think there is a way to identify an untagged object. There is 
nothing that is passed in the callback to allow that.


igor


On Nov 28, 2018, at 1:32 PM, dean.l...@oracle.com 
 wrote:


I'm guessing Graal creates Constant boxes of the individual fields 
and not of the test objects?  If so, can't we fix the matching so 
that it checks that all fields of an object match?  I guess that 
would mean moving the "expected" and "found" counts up from fields[] 
to objects_info[].


dl

On 11/28/18 1:13 PM, Igor Veresov wrote:
When doing heap iteration with JVMTI the way the object identity is 
tracked is by tagging. This particular test checks if it  can 
observe an untagged object. Since there is no way to truly track 
the identity of an untagged object the test validates the identity 
by checking the value of the fields of such object. When being 
compiled with Graal there are objects produced (such as Constant 
boxes) that have field values that are equal to the expected values 
for the fields in UntaggedClass. During the subsequent heap 
iteration these objects are reported to the test and are treated as 
if they were instances of UntaggedClass.


The fix is to relax the test requirement and allow (for the 
untagged case) the number of the object found during iteration to 
be more than expected.


Webrev: http://cr.openjdk.java.net/~iveresov/8193577/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8193577

igor

Re: RFR JDK-8195639: [Graal] nsk/jvmti/PopFrame/popframe009 fail with Graal in -Xcomp

[dean . long]

I guess even with -Xcomp, there are still compiles triggered by profiled 
code, otherwise I don't see why changing policy()->event() helps.


Another problem I see with this change is that it will make the real 
problem harder to reproduce.


dl

On 11/28/18 12:36 PM, Alex Menkov wrote:

Hi guys,

I was able to reproduce the issue only with Graal + -Xcomp and only 
with debug build (I tried Graal + -Xint, Graal without Xcomp/Xint, 
just -Xcomp (i.e. C1 is used)).


--alex

On 11/28/2018 01:27, serguei.spit...@oracle.com wrote:

On 11/28/18 01:07, dean.l...@oracle.com wrote:

OK, reconsidering solution 1, I have a couple more questions:

The bug report says the problem is reproducible with -Xcomp, but I 
don't see the policy()->event() path taken with -Xcomp. Instead, 
-Xcomp uses CompilationPolicy::compile_if_required, so I don't see 
how Solution 1 fixes the problem with -Xcomp.


> CompilerRuntime always calls policy()->event with CompLevel == 
CompLevel_aot.


As far as I can tell, CompLevel_aot is only used when there is 
active AOT code.  Is this an AOT-specific problem?


Tracing showed that the CompLevel_aot is always used for JVMCI which 
is pretty confusing.
Most likely, it is because now the AOT is enabled by default (is it 
correct?).

I'm not sure it actually has anything to do with the AOT mode.



  I would expect there to be a problem with c1 as well.


Not sure about the c1.
I hope, Alex could comment on this tomorrow.


Thanks,
Serguei


dl

On 11/27/18 3:38 PM, serguei.spit...@oracle.com wrote:

Good summary with a list of solutions below!

On 11/27/18 2:50 PM, dean.l...@oracle.com wrote:

Let me list the proposed solutions:

1. short-circuit compiles in CompilationPolicy::event in 
is_interp_only_mode

2. detect is_interp_only_mode in resolve stubs and force a c2i call
3. don't offer a JVMTI suspend point in resolve stubs (or 
JavaCallWrapper)


Solution 1 penalizes other threads, and don't prevent other 
threads from compiling the method, so while it may help a little, 
it's incomplete.


I doubt, the Solution 1 penalizes other threads.
First, it is an important pretty frequent/common case that allows 
to save on compilations.
Second, there is no point to compile methods on a thread executed 
in interp_only_mode.
Also, there is no big advantage to compile methods even for other 
threads

when an active debugging (e.g. single stepping) is in progress.
Third, this would make it consistent with what the the interpreter 
is doing.


So, I think, the Solution 1 is needed independently of other 
solutions we take.

My suggestion is to apply it in the JDK-8195639 bug fix.

Solutions 1 and 2 allow the thread to resume in a different method 
(the callee method), causing other problems.


Agreed.

With Solution 3, the frame we suspend is the same as the frame we 
end up in after the resume, so I believe it solves all the problems.

IMHO this is the correct solution to pursue.


I agree in general.
This solution has some risks involved as it impacts a lot of code 
including platform-independent.
It is also going to fix the other bugs: JDK-8195635, JDK-8214093, 
JDK-8207013 (not sure about all of them yet).


My suggestion is to pursue it as the JDK-8195635 fix.
Could you, please, confirm if works for you?

Also, we have no consensus yet on the Solution 1.

Thanks,
Serguei


dl

On 11/27/18 2:19 PM, serguei.spit...@oracle.com wrote:

Hi Dean,

You also asked the questions:

> Doesn't is_interp_only_mode() only apply to the current thread?

Yes it does.


> I don't think it's safe to assume no compiles will happen in 
other threads,
> or that a method call target is not already compiled, because 
as far

> as I can tell, JVMTI only deoptimizes the active frames.

I agree with it.
However, compilations on the thread with the interp_only_mode 
enabled is the most important case.

Disabling compilations such thread improves testing stability.

> The bug report describes a case where the caller has been 
deoptimized
> while resolving a call.  I don't see how this fix prevents the 
target

> from being a previously compiled method.
> But we do need to make sure not to call into compiled code, so 
I think
> the fix needs to be in code like 
SharedRuntime::resolve_static_call_C(),

> where it returns get_c2i_entry() if is_interp_only_mode() is true.

Thank you for making this point.

It would be nice to attack this as well.
We can try to investigate this approach further.
One problem is that there are more cases like resolve_static_call_C,
for instance, resolve_virtual_call_C and resolve_opt_virtual_call_C.
We may need to fix the same in other places, like 
JavaCallWrapper::JavaCallWrapper.


We can start from fixing it in the resolve_static_call_C.
If it is successful then continue this effort for other cases as 
well.


What do you think?

Thanks,
Serguei


On 11/27/18 1:01 PM, Alex Menkov wrote:

Hi Dean,

Thank you for the feedback and for the comments in Jira.

>> How does this solve the problem of

Re: RFR(XS) 8193577: nsk/jvmti/IterateThroughHeap/filter-tagged fails with Graal in Xcomp mode

[dean . long]

I'm guessing Graal creates Constant boxes of the individual fields and 
not of the test objects?  If so, can't we fix the matching so that it 
checks that all fields of an object match?  I guess that would mean 
moving the "expected" and "found" counts up from fields[] to objects_info[].


dl

On 11/28/18 1:13 PM, Igor Veresov wrote:

When doing heap iteration with JVMTI the way the object identity is tracked is 
by tagging. This particular test checks if it  can observe an untagged object. 
Since there is no way to truly track the identity of an untagged object the 
test validates the identity by checking the value of the fields of such object. 
When being compiled with Graal there are objects produced (such as Constant 
boxes) that have field values that are equal to the expected values for the 
fields in UntaggedClass. During the subsequent heap iteration these objects are 
reported to the test and are treated as if they were instances of UntaggedClass.

The fix is to relax the test requirement and allow (for the untagged case) the 
number of the object found during iteration to be more than expected.

Webrev: http://cr.openjdk.java.net/~iveresov/8193577/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8193577

igor

Re: RFR JDK-8195639: [Graal] nsk/jvmti/PopFrame/popframe009 fail with Graal in -Xcomp

[dean . long]

OK, reconsidering solution 1, I have a couple more questions:

The bug report says the problem is reproducible with -Xcomp, but I don't 
see the policy()->event() path taken with -Xcomp.  Instead, -Xcomp uses 
CompilationPolicy::compile_if_required, so I don't see how Solution 1 
fixes the problem with -Xcomp.


> CompilerRuntime always calls policy()->event with CompLevel == 
CompLevel_aot.


As far as I can tell, CompLevel_aot is only used when there is active 
AOT code.  Is this an AOT-specific problem?  I would expect there to be 
a problem with c1 as well.


dl

On 11/27/18 3:38 PM, serguei.spit...@oracle.com wrote:

Good summary with a list of solutions below!

On 11/27/18 2:50 PM, dean.l...@oracle.com wrote:

Let me list the proposed solutions:

1. short-circuit compiles in CompilationPolicy::event in 
is_interp_only_mode

2. detect is_interp_only_mode in resolve stubs and force a c2i call
3. don't offer a JVMTI suspend point in resolve stubs (or 
JavaCallWrapper)


Solution 1 penalizes other threads, and don't prevent other threads 
from compiling the method, so while it may help a little, it's 
incomplete.


I doubt, the Solution 1 penalizes other threads.
First, it is an important pretty frequent/common case that allows to 
save on compilations.
Second, there is no point to compile methods on a thread executed in 
interp_only_mode.

Also, there is no big advantage to compile methods even for other threads
when an active debugging (e.g. single stepping) is in progress.
Third, this would make it consistent with what the the interpreter is 
doing.


So, I think, the Solution 1 is needed independently of other solutions 
we take.

My suggestion is to apply it in the JDK-8195639 bug fix.

Solutions 1 and 2 allow the thread to resume in a different method 
(the callee method), causing other problems.


Agreed.

With Solution 3, the frame we suspend is the same as the frame we end 
up in after the resume, so I believe it solves all the problems.

IMHO this is the correct solution to pursue.


I agree in general.
This solution has some risks involved as it impacts a lot of code 
including platform-independent.
It is also going to fix the other bugs: JDK-8195635, JDK-8214093, 
JDK-8207013 (not sure about all of them yet).


My suggestion is to pursue it as the JDK-8195635 fix.
Could you, please, confirm if works for you?

Also, we have no consensus yet on the Solution 1.

Thanks,
Serguei


dl

On 11/27/18 2:19 PM, serguei.spit...@oracle.com wrote:

Hi Dean,

You also asked the questions:

> Doesn't is_interp_only_mode() only apply to the current thread?

Yes it does.


> I don't think it's safe to assume no compiles will happen in other 
threads,

> or that a method call target is not already compiled, because as far
> as I can tell, JVMTI only deoptimizes the active frames.

I agree with it.
However, compilations on the thread with the interp_only_mode 
enabled is the most important case.

Disabling compilations such thread improves testing stability.

> The bug report describes a case where the caller has been deoptimized
> while resolving a call.  I don't see how this fix prevents the target
> from being a previously compiled method.
> But we do need to make sure not to call into compiled code, so I 
think
> the fix needs to be in code like 
SharedRuntime::resolve_static_call_C(),

> where it returns get_c2i_entry() if is_interp_only_mode() is true.

Thank you for making this point.

It would be nice to attack this as well.
We can try to investigate this approach further.
One problem is that there are more cases like resolve_static_call_C,
for instance, resolve_virtual_call_C and resolve_opt_virtual_call_C.
We may need to fix the same in other places, like 
JavaCallWrapper::JavaCallWrapper.


We can start from fixing it in the resolve_static_call_C.
If it is successful then continue this effort for other cases as well.

What do you think?

Thanks,
Serguei


On 11/27/18 1:01 PM, Alex Menkov wrote:

Hi Dean,

Thank you for the feedback and for the comments in Jira.

>> How does this solve the problem of
>> HotSpotJVMCIRuntime.adjustCompilationLevel being called?

It doesn't. It fixes another issue.
The test suspend test thread and ensures that top frame is a 
"fibonachi" method. Then it turns SingleStep on, does PopFrame and 
resumes the thread.
The problem is the thread continues execution of compiled code 
(ignoring SingleStep) and we get SindleStep event only when 
adjustCompilationLevel method is called (it's interpreted code).


There are several other bugs which are (most likely) caused by 
suspend during call setup (JDK-8195635, JDK-8214093, JDK-8207013)


--alex

On 11/27/2018 01:39, serguei.spit...@oracle.com wrote:

Hi Dean,

Thank you a lot for looking at this!
Just a couple of points from me (it is up to Alex to provide a 
full answer).



I think, Alex in this RFR missed to tell that we knew about this 
issue that an incorrect frame will be popped.
But after some discussion we decided to 

Re: RFR JDK-8195639: [Graal] nsk/jvmti/PopFrame/popframe009 fail with Graal in -Xcomp

[dean . long]

Let me list the proposed solutions:

1. short-circuit compiles in CompilationPolicy::event in is_interp_only_mode
2. detect is_interp_only_mode in resolve stubs and force a c2i call
3. don't offer a JVMTI suspend point in resolve stubs (or JavaCallWrapper)

Solution 1 penalizes other threads, and don't prevent other threads from 
compiling the method, so while it may help a little, it's incomplete.
Solutions 1 and 2 allow the thread to resume in a different method (the 
callee method), causing other problems.
With Solution 3, the frame we suspend is the same as the frame we end up 
in after the resume, so I believe it solves all the problems.

IMHO this is the correct solution to pursue.

dl

On 11/27/18 2:19 PM, serguei.spit...@oracle.com wrote:

Hi Dean,

You also asked the questions:

> Doesn't is_interp_only_mode() only apply to the current thread?

Yes it does.


> I don't think it's safe to assume no compiles will happen in other 
threads,

> or that a method call target is not already compiled, because as far
> as I can tell, JVMTI only deoptimizes the active frames.

I agree with it.
However, compilations on the thread with the interp_only_mode enabled 
is the most important case.

Disabling compilations such thread improves testing stability.

> The bug report describes a case where the caller has been deoptimized
> while resolving a call.  I don't see how this fix prevents the target
> from being a previously compiled method.
> But we do need to make sure not to call into compiled code, so I think
> the fix needs to be in code like 
SharedRuntime::resolve_static_call_C(),

> where it returns get_c2i_entry() if is_interp_only_mode() is true.

Thank you for making this point.

It would be nice to attack this as well.
We can try to investigate this approach further.
One problem is that there are more cases like resolve_static_call_C,
for instance, resolve_virtual_call_C and resolve_opt_virtual_call_C.
We may need to fix the same in other places, like 
JavaCallWrapper::JavaCallWrapper.


We can start from fixing it in the resolve_static_call_C.
If it is successful then continue this effort for other cases as well.

What do you think?

Thanks,
Serguei


On 11/27/18 1:01 PM, Alex Menkov wrote:

Hi Dean,

Thank you for the feedback and for the comments in Jira.

>> How does this solve the problem of
>> HotSpotJVMCIRuntime.adjustCompilationLevel being called?

It doesn't. It fixes another issue.
The test suspend test thread and ensures that top frame is a 
"fibonachi" method. Then it turns SingleStep on, does PopFrame and 
resumes the thread.
The problem is the thread continues execution of compiled code 
(ignoring SingleStep) and we get SindleStep event only when 
adjustCompilationLevel method is called (it's interpreted code).


There are several other bugs which are (most likely) caused by 
suspend during call setup (JDK-8195635, JDK-8214093, JDK-8207013)


--alex

On 11/27/2018 01:39, serguei.spit...@oracle.com wrote:

Hi Dean,

Thank you a lot for looking at this!
Just a couple of points from me (it is up to Alex to provide a full 
answer).



I think, Alex in this RFR missed to tell that we knew about this 
issue that an incorrect frame will be popped.

But after some discussion we decided to file a separate issue on this.
Alex wanted to create a good stand-alone test case demonstrating 
this problem before filing it.


Now, as I can see, the JDK-8195635 
 looks very close 
to a bug that we wanted to file.
The only difference is that our scenario includes the 
SharedRuntime::resolve_static_call_C instead of the 
JavaCallWrapper::JavaCallWrapper as a helper for Java method 
invocation.
If I understand corrctly (Alex will fix me if needed), the jtreg 
test we used to chase this issue did not catch a problem with the 
HotSpotJVMCIRuntime.adjustCompilationLevel.



The suggested fix was to fix the mismatch in the 
TieredThresholdPolicy::even with the comment in the interpreter code:
nmethod* InterpreterRuntime::frequency_counter_overflow(JavaThread* 
thread, address branch_bcp) {

 . . .
   if (nm != NULL && thread->is_interp_only_mode()) {
 // Normally we never get an nm if is_interp_only_mode() is 
true, because
 // policy()->event has a check for this and won't compile the 
method when
 // true. However, it's possible for is_interp_only_mode() to 
become true
 // during the compilation. We don't want to return the nm in 
that case

 // because we want to continue to execute interpreted.
 nm = NULL;
   }

 > So I think the fix needs to be in code like 
SharedRuntime::resolve_static_call_C(),

 > where it returns get_c2i_entry() if is_interp_only_mode() is true.

I'm not sure, the adding this condition and returning the 
get_c2i_entry() is always correct.

We need some help from the Compiler team to make it right.

BTW, the interp_only_mode has been enabled only when some 
interp_only events are enabled.

It is not set by 

Re: RFR JDK-8195639: [Graal] nsk/jvmti/PopFrame/popframe009 fail with Graal in -Xcomp

[dean . long]

How does this solve the problem of 
HotSpotJVMCIRuntime.adjustCompilationLevel being called?


I don't think this fix is the right approach.  Doesn't 
is_interp_only_mode() only apply to the current thread?  I don't think 
it's safe to assume no compiles will happen in other threads, or that a 
method call target is not already compiled, because as far as I can 
tell, JVMTI only deoptimizes the active frames.  The bug report 
describes a case where the caller has been deoptimized while resolving a 
call.  I don't see how this fix prevents the target from being a 
previously compiled method.  But we do need to make sure not to call 
into compiled code, so I think the fix needs to be in code like 
SharedRuntime::resolve_static_call_C(), where it returns get_c2i_entry() 
if is_interp_only_mode() is true.  However, there is still another 
problem.  By allowing JVMTI to suspend the thread during call setup, but 
reporting the frame as still in the caller instead of the callee, we 
confuse JVMTI into thinking that execution will resume in the caller 
instead of the callee.  We may want to restrict where we offer JVMTI 
suspend points, and not offer a JVMTI suspend point in 
SharedRuntime::resolve_static_call_C and friends at all.


dl


On 11/26/18 11:14 AM, Alex Menkov wrote:

Hi all,

Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8195639
webrev:
http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/

Description:
The test suspends a thread, turns on single stepping and then calls 
PopFrame. SingleStep event is expected as soon as the thread is 
resumed and PopFrame is processed (so we have call stack with the 
depth 1 less than it was before PopFrame). Instead SingleStep event is 
received with much deeper call stack (and PopFrame processes wrong 
frame).
Research shown that this is caused by missed deoptimization of the 
current frame.
As far as I understand CompilationPolicy::event should handle the case 
when the thread has is_interp_only_mode() set, but 
TieredThresholdPolicy::event checks this only then CompLevel is 
CompLevel_none.
CompilerRuntime always calls policy()->event with CompLevel == 
CompLevel_aot.


The fix looks quite simple, but I'd appreciate feedback from runtime 
and compiler teams as I'm not sure I completely understand all the 
details of the "PopFrame dance".


--alex

Re: JNI - question about jmethodid values.

[dean . long]

Doesn't the spec say it's the caller's responsibility to keep the class 
from being unloaded?


"
A field or method ID does not prevent the VM from unloading the class 
from which the ID has been derived. After the class is unloaded, the 
method or field ID becomes invalid. The native code, therefore, must 
make sure to:


 * keep a live reference to the underlying class, or
 * recompute the method or field ID

if it intends to use a method or field ID for an extended period of time.

The JNI does not impose any restrictions on how field and method IDs are 
implemented internally.


"


however, then the JVMTI spec has errors like 
|JVMTI_ERROR_INVALID_METHODID| 
 
that seem to imply that the JVM can (or must?) detect an invalid methodid.


dl

On 11/26/18 11:33 AM, Thomas Stüfe wrote:

Hey JC,

On Mon, Nov 26, 2018 at 8:15 PM JC Beyler  wrote:

Hi all,

Just added my two cents on one comment:

On Mon, Nov 26, 2018 at 5:00 AM Thomas Stüfe  wrote:

Hi Peter,

On Mon, Nov 26, 2018 at 1:02 PM Peter Hull  wrote:

Hi Thomas,
Thank you very much for the detailed explanation. For your
information, the relevant NetBeans bug is
https://issues.apache.org/jira/browse/NETBEANS-1428

On Sat, Nov 24, 2018 at 3:41 PM Thomas Stüfe  wrote:

A jmethodid is a pointer to malloc'ed memory.

OK. Just in case I haven't understood it - does this mean a jmethodid,
once 'created', won't change (after garbage collection or whatever)?

yes. It lives on, unchanged, forever. Even if the associated class is
unloaded. That must be since outside JNI/JVMTI code may call in with
outdated jmethodids which we must be able to handle gracefully.


This is the current design and most likely will remain for quite a bit but it 
is not defined by the SPEC to be like this so, in essence, it is an 
implementation detail that could be not true in future releases ;-)

I politely disagree :)

JNI spec says that you have a function like this:

jmethodID GetMethodID(..)

returning a jmethodid which you can then use in subsequent JNI
functions. These functions are required to behave in a predictable way
if the jmethodid has gotten invalid. The spec has no way to inform the
caller if a jmethodid has gotten invalid by class unloading. Since the
jmethodid is handed up to the caller and stored by him, there is no
way to change its value on the fly either.

So even though the spec does not specifically say that jmethodid lives
forever, it is laid out in a way that prevents jmethodids from ever
becoming invalid, and from changing their numerical value. This can of
course change, but not without changing the JNI spec.

I think my point is that Peter can rely on this behavior without fear
that it will change in the future without him noticing. Before this
behavior changes, the JNI spec would have to change.

Cheers, Thomas








Thanks,
Jc


But it is not guaranteed to work. I would probably rather use a
hashmap or similar.

I need to look at the implications on more detail but think it would
make sense to use long/jlong instead of int/jint on all platforms; the
extra memory use shouldn't be a problem. I think the IDs are just
stored on the Java side and used to get the method name and signature
later. That should be a loss-free cast, shouldn't it?

Sure.


If this is
true, this 4x30bit assumption may actually have worked before jdk8,
since the java heap is allocated as one continuous space, with the
PermGen clustered in one part of it.

Indeed we did only start to get crashes on JDK9 and later (only
observed on Windows, macOS seems OK and other platforms have not been
tested)

Yours sincerely,
Peter

Cheers, Thomas



--

Thanks,
Jc

Re: RFR 8203174: [Graal] JDI tests fail with Unexpected exception: com.sun.jdi.ObjectCollectedException

[dean . long]

I see, eventually it should stop waiting and timeout.  That seems fine.

dl

On 11/9/18 12:26 PM, Daniil Titov wrote:

Hi Dean,

The blocking issue  for suspended JVMCI compiler thread in case of Graal and -XComp was 
recently solved in  JDK-8195627 " Graal] 
nsk/jdi/VirtualMachine/redefineClasses/redefineclasses026 hangs with Graal in Xcomp 
mode" .  Could you please say do you think it will not be sufficient for the case 
you described?

Just in case please find below the links to Jira and review thread for 
JDK-8195627:
Review thread :  
http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-October/025764.html
Jira issue: https://bugs.openjdk.java.net/browse/JDK-8195627

Thanks,
Daniil

On 11/9/18, 12:14 PM, "serguei.spit...@oracle.com" 
 wrote:

 We need to understand/formulate what requirements/limits the Graal
 compiler has to follow and then negotiate it with the Compiler team.
 I feel it is not a good idea to work around these issues to make the
 tests to pass.
 Sorry that I've lost track of the discussion - will need to re-read the
 email thread thoroughly.
 
 Thanks,

 Serguei
 
 
 On 11/9/18 11:42, Chris Plummer wrote:

 > This sounds like a general issue with debugging and graal. Debuggers
 > expect to be able to suspend all threads, and then resume just the
 > ones they want to see executed. You're saying there are situations in
 > which the resumed thread will end up blocking on the suspended
 > compiler thread(s).
 >
 > Chris
 >
 >
 > On 11/9/18 10:19 AM, dean.l...@oracle.com wrote:
 >> With OSR, a compile can be initiated in a for loop, so I'd say this
 >> is still not safe if the compile can be blocking, which happens if:
 >>
 >> bool is_blocking = !directive->BackgroundCompilationOption ||
 >> CompileTheWorld || ReplayCompiles;
 >>
 >> So that probably means the test cannot be run with -Xcomp.
 >>
 >> dl
 >>
 >> On 11/8/18 3:12 PM, Daniil Titov wrote:
 >>> The proposed fix for this case is to suspend VM while the body of
 >>> the "for" loop (lines 150 - 356) is executed while ensuring that for
 >>> line 151 ( line = pipe.printlln()) VM is temporary resumed ( line
 >>> numbers are given for an original version of
 >>> 
test/hotspot/jtreg/vmTestbase/nsk/jdi/ArrayType/newInstance/newinstance001.java)
 >>
 >
 >
 
 

Re: RFR 8203174: [Graal] JDI tests fail with Unexpected exception: com.sun.jdi.ObjectCollectedException

[dean . long]

With OSR, a compile can be initiated in a for loop, so I'd say this is 
still not safe if the compile can be blocking, which happens if:


    bool is_blocking = !directive->BackgroundCompilationOption || 
CompileTheWorld || ReplayCompiles;


So that probably means the test cannot be run with -Xcomp.

dl

On 11/8/18 3:12 PM, Daniil Titov wrote:

The proposed fix for this case is to suspend VM while the body of the "for" 
loop (lines 150 - 356) is executed while ensuring that for line 151 ( line = 
pipe.printlln()) VM is temporary resumed ( line numbers are given for an original version 
of  test/hotspot/jtreg/vmTestbase/nsk/jdi/ArrayType/newInstance/newinstance001.java)

Re: RFR 8203174: [Graal] JDI tests fail with Unexpected exception: com.sun.jdi.ObjectCollectedException

[dean . long]

I don't think suspending compiler threads is safe, especially if 
compiles are blocking/synchronous.


dl

On 11/2/18 4:29 PM, Daniil Titov wrote:

Please review the change that fixes several tests failing with 
com.sun.jdi.ObjectCollectedException when running with Graal.

There main problem here is that with Graal on JVMCI Compiler threads in the target VM 
create a lot of objects and sporadically trigger GC that results in the objects created 
in the target VM in the tests being GCed before the tests complete. The other problem is 
that for some classes the tests use, e.g. "java.lang.String",  there is already 
a huge number of instances created by JVMCI threads.

The fix suspends target VM to prevent JVMCI compiler threads from creating new objects 
during the tests execution. In cases when IOPipe is used for communication between the 
test and the debuggee the fix suspends all threads but the main rather than suspending 
the VM. The fix also filters out the checks for some test classes (e.g. 
"java.lang.String") in cases when the target VM is run with JVMCI compiler 
options on.

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

Thanks,
Daniil

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/23/18 11:04 PM, Mandy Chung wrote:



On 10/23/18 10:34 PM, David Holmes wrote:

On 24/10/2018 8:30 AM, dean.l...@oracle.com wrote:

On 10/23/18 2:51 PM, David Holmes wrote:

Hi Dean,

On 24/10/2018 4:05 AM, dean.l...@oracle.com wrote:

On 10/23/18 9:46 AM, dean.l...@oracle.com wrote:

On 10/22/18 3:31 PM, David Holmes wrote:
Sorry Dean I'm concerned about a thread termination bottleneck 
with this. A simple microbenchmark that creates 500,000 threads 
that have to run and terminate, shows a 15+% slowdown on my 
Linux box. I tried to find some kind of real benchmarks that 
covers thread termination but couldn't see anything specific.


Can you at least run this through our performance system to see 
if any of the regular benchmarks are affected.




OK, but even if the regular benchmarks don't show a difference, 
I'd feel better if microbenchmarks were not affected.  What if I 
go back to the original approach and add locking:


   static jlong get_live_thread_count()    { MutexLocker 
mu(Threads_lock); return _live_threads_count->get_value() - 
_exiting_threads_count; }
   static jlong get_daemon_thread_count()  { MutexLocker 
mu(Threads_lock); return _daemon_threads_count->get_value() - 
_exiting_daemon_threads_count; }


along with the other cleanups around is_daemon and is_hidden_thread?



Some micro-benchmarks like SecureRandomBench show a regression 
with webrev.7.  I could go back to webrev.5 and then we shouldn't 
need any locking in the get_*() functions.


I don't see version 5 discussed but I took a look and it seems okay. 


Mandy had questions about the asserts in .5, and it seemed like we 
could just set the perf counters to the same value as the atomic 
counters, which resulted in .6.  I think the only problem with .6 is 
that I set the perf counters in decrement_thread_counts without the 
lock.  If I "sync" the perf counters to the atomic counters only in 
add_thread and remove_thread, with the lock, then it's about the 
same as .5, but without the asserts and parallel inc/dec.  If anyone 
likes the sound of that, I can send out a new webrev.  Or we can go 
with webrev.5.


I'm not sure what the concern was with the asserts - if they mis-fire 
we'll know soon enough. So I'm okay with .5


My only query with that version is that it appears the actual 
perfCounters no longer get read by anything - is that the case?




There does seem to be code that references them, through their name 
string.  That's a difference interface that I'm not familiar with, 
so I didn't want to break it.


Right - they can be accessed directly through other means. I was 
concerned that the perfCounter was out of sync with 
get_live_thread-count() but that's already the case so not an issue.


If all tests and benchmarks are happy I say go with version .5



I have no objection to version .5 if most people prefer that.  My 
comment was that I don't think the asserts are necessary.




OK, I'll rerun some performance benchmarks and push .5 if the results 
look OK.  David, can you send me your micro-benchmark?

Thanks for the reviews!

dl

Mandy

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/23/18 2:51 PM, David Holmes wrote:

Hi Dean,

On 24/10/2018 4:05 AM, dean.l...@oracle.com wrote:

On 10/23/18 9:46 AM, dean.l...@oracle.com wrote:

On 10/22/18 3:31 PM, David Holmes wrote:
Sorry Dean I'm concerned about a thread termination bottleneck with 
this. A simple microbenchmark that creates 500,000 threads that 
have to run and terminate, shows a 15+% slowdown on my Linux box. I 
tried to find some kind of real benchmarks that covers thread 
termination but couldn't see anything specific.


Can you at least run this through our performance system to see if 
any of the regular benchmarks are affected.




OK, but even if the regular benchmarks don't show a difference, I'd 
feel better if microbenchmarks were not affected.  What if I go back 
to the original approach and add locking:


   static jlong get_live_thread_count()    { MutexLocker 
mu(Threads_lock); return _live_threads_count->get_value() - 
_exiting_threads_count; }
   static jlong get_daemon_thread_count()  { MutexLocker 
mu(Threads_lock); return _daemon_threads_count->get_value() - 
_exiting_daemon_threads_count; }


along with the other cleanups around is_daemon and is_hidden_thread?



Some micro-benchmarks like SecureRandomBench show a regression with 
webrev.7.  I could go back to webrev.5 and then we shouldn't need any 
locking in the get_*() functions.


I don't see version 5 discussed but I took a look and it seems okay. 


Mandy had questions about the asserts in .5, and it seemed like we could 
just set the perf counters to the same value as the atomic counters, 
which resulted in .6.  I think the only problem with .6 is that I set 
the perf counters in decrement_thread_counts without the lock.  If I 
"sync" the perf counters to the atomic counters only in add_thread and 
remove_thread, with the lock, then it's about the same as .5, but 
without the asserts and parallel inc/dec.  If anyone likes the sound of 
that, I can send out a new webrev.  Or we can go with webrev.5.


My only query with that version is that it appears the actual 
perfCounters no longer get read by anything - is that the case?




There does seem to be code that references them, through their name 
string.  That's a difference interface that I'm not familiar with, so I 
didn't want to break it.


dl


Thanks,
David


dl


dl


Thanks,
David

On 20/10/2018 1:28 PM, dean.l...@oracle.com wrote:

On 10/18/18 6:12 PM, Mandy Chung wrote:



On 10/18/18 12:27 AM, David Holmes wrote:

Hi Dean,

On 18/10/2018 2:06 PM, dean.l...@oracle.com wrote:


You're right, I missed that.  I think the right thing to do is 
call current_thread_exiting while holding the Threads_lock.
Then we can get rid of the parallel atomic counters. So, here's 
one more try:


http://cr.openjdk.java.net/~dlong/8021335/webrev.7/


Okay that is the simple and obvious solution that doesn't 
require split counts. So I have to ask Mandy if she recalls why 
this approach wasn't taken 15 years ago when the exit counts 
were added as part of:




It has been so long.  I think it's likely an oversight.

https://bugs.openjdk.java.net/browse/JDK-4530538 ?

Does taking the Threads_lock here cost too much and cause a 
thread termination bottleneck?


If the contention on Threads_lock is not high (that seems to me), 
it should be okay.   I'm not close to the VM implementation (lot 
of changes since then) and I don't have a definitive answer 
unless I study the code closely.   You and others have a better 
judgement on this.


AFAICT the change is okay.



Thanks Mandy.  David, OK to push?

dl


Mandy

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/23/18 9:46 AM, dean.l...@oracle.com wrote:

On 10/22/18 3:31 PM, David Holmes wrote:
Sorry Dean I'm concerned about a thread termination bottleneck with 
this. A simple microbenchmark that creates 500,000 threads that have 
to run and terminate, shows a 15+% slowdown on my Linux box. I tried 
to find some kind of real benchmarks that covers thread termination 
but couldn't see anything specific.


Can you at least run this through our performance system to see if 
any of the regular benchmarks are affected.




OK, but even if the regular benchmarks don't show a difference, I'd 
feel better if microbenchmarks were not affected.  What if I go back 
to the original approach and add locking:


   static jlong get_live_thread_count()    { MutexLocker 
mu(Threads_lock); return _live_threads_count->get_value() - 
_exiting_threads_count; }
   static jlong get_daemon_thread_count()  { MutexLocker 
mu(Threads_lock); return _daemon_threads_count->get_value() - 
_exiting_daemon_threads_count; }


along with the other cleanups around is_daemon and is_hidden_thread?



Some micro-benchmarks like SecureRandomBench show a regression with 
webrev.7.  I could go back to webrev.5 and then we shouldn't need any 
locking in the get_*() functions.


dl


dl


Thanks,
David

On 20/10/2018 1:28 PM, dean.l...@oracle.com wrote:

On 10/18/18 6:12 PM, Mandy Chung wrote:



On 10/18/18 12:27 AM, David Holmes wrote:

Hi Dean,

On 18/10/2018 2:06 PM, dean.l...@oracle.com wrote:


You're right, I missed that.  I think the right thing to do is 
call current_thread_exiting while holding the Threads_lock.
Then we can get rid of the parallel atomic counters. So, here's 
one more try:


http://cr.openjdk.java.net/~dlong/8021335/webrev.7/


Okay that is the simple and obvious solution that doesn't require 
split counts. So I have to ask Mandy if she recalls why this 
approach wasn't taken 15 years ago when the exit counts were added 
as part of:




It has been so long.  I think it's likely an oversight.

https://bugs.openjdk.java.net/browse/JDK-4530538 ?

Does taking the Threads_lock here cost too much and cause a thread 
termination bottleneck?


If the contention on Threads_lock is not high (that seems to me), 
it should be okay.   I'm not close to the VM implementation (lot of 
changes since then) and I don't have a definitive answer unless I 
study the code closely.   You and others have a better judgement on 
this.


AFAICT the change is okay.



Thanks Mandy.  David, OK to push?

dl


Mandy

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/22/18 3:31 PM, David Holmes wrote:
Sorry Dean I'm concerned about a thread termination bottleneck with 
this. A simple microbenchmark that creates 500,000 threads that have 
to run and terminate, shows a 15+% slowdown on my Linux box. I tried 
to find some kind of real benchmarks that covers thread termination 
but couldn't see anything specific.


Can you at least run this through our performance system to see if any 
of the regular benchmarks are affected.




OK, but even if the regular benchmarks don't show a difference, I'd feel 
better if microbenchmarks were not affected.  What if I go back to the 
original approach and add locking:


   static jlong get_live_thread_count()    { MutexLocker 
mu(Threads_lock); return _live_threads_count->get_value() - 
_exiting_threads_count; }
   static jlong get_daemon_thread_count()  { MutexLocker 
mu(Threads_lock); return _daemon_threads_count->get_value() - 
_exiting_daemon_threads_count; }


along with the other cleanups around is_daemon and is_hidden_thread?

dl


Thanks,
David

On 20/10/2018 1:28 PM, dean.l...@oracle.com wrote:

On 10/18/18 6:12 PM, Mandy Chung wrote:



On 10/18/18 12:27 AM, David Holmes wrote:

Hi Dean,

On 18/10/2018 2:06 PM, dean.l...@oracle.com wrote:


You're right, I missed that.  I think the right thing to do is 
call current_thread_exiting while holding the Threads_lock.
Then we can get rid of the parallel atomic counters.  So, here's 
one more try:


http://cr.openjdk.java.net/~dlong/8021335/webrev.7/


Okay that is the simple and obvious solution that doesn't require 
split counts. So I have to ask Mandy if she recalls why this 
approach wasn't taken 15 years ago when the exit counts were added 
as part of:




It has been so long.  I think it's likely an oversight.

https://bugs.openjdk.java.net/browse/JDK-4530538 ?

Does taking the Threads_lock here cost too much and cause a thread 
termination bottleneck?


If the contention on Threads_lock is not high (that seems to me), it 
should be okay.   I'm not close to the VM implementation (lot of 
changes since then) and I don't have a definitive answer unless I 
study the code closely.   You and others have a better judgement on 
this.


AFAICT the change is okay.



Thanks Mandy.  David, OK to push?

dl


Mandy

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/18/18 6:12 PM, Mandy Chung wrote:



On 10/18/18 12:27 AM, David Holmes wrote:

Hi Dean,

On 18/10/2018 2:06 PM, dean.l...@oracle.com wrote:


You're right, I missed that.  I think the right thing to do is call 
current_thread_exiting while holding the Threads_lock.
Then we can get rid of the parallel atomic counters.  So, here's one 
more try:


http://cr.openjdk.java.net/~dlong/8021335/webrev.7/


Okay that is the simple and obvious solution that doesn't require 
split counts. So I have to ask Mandy if she recalls why this approach 
wasn't taken 15 years ago when the exit counts were added as part of:




It has been so long.  I think it's likely an oversight.

https://bugs.openjdk.java.net/browse/JDK-4530538 ?

Does taking the Threads_lock here cost too much and cause a thread 
termination bottleneck?


If the contention on Threads_lock is not high (that seems to me), it 
should be okay.   I'm not close to the VM implementation (lot of 
changes since then) and I don't have a definitive answer unless I 
study the code closely.   You and others have a better judgement on this.


AFAICT the change is okay.



Thanks Mandy.  David, OK to push?

dl


Mandy

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/17/18 7:07 PM, David Holmes wrote:

Hi Dean,

This still seems racy to me. We increment all counts under the 
Threads_lock but we still decrement without the Threads_lock. So we 
can lose updates to the perfCounters.


 117   _total_threads_count->inc();
 118   Atomic::inc(&_atomic_threads_count);
 119   int count = _atomic_threads_count;
<= context switch here
 120   _live_threads_count->set_value(count);

If a second thread now exits while the above thread is descheduled, it 
will decrement _atomic_threads_count and _live_threads_count, but when 
the first thread resumes at line 120 above we will set 
_live_threads_count to the wrong value!


You can't maintain two counters in sync without all changes using 
locking across both.




You're right, I missed that.  I think the right thing to do is call 
current_thread_exiting while holding the Threads_lock.
Then we can get rid of the parallel atomic counters.  So, here's one 
more try:


    http://cr.openjdk.java.net/~dlong/8021335/webrev.7/

dl


David



On 18/10/2018 8:18 AM, dean.l...@oracle.com wrote:

On 10/17/18 2:38 PM, Mandy Chung wrote:



On 10/17/18 2:13 PM, dean.l...@oracle.com wrote:

On 10/17/18 1:41 PM, Mandy Chung wrote:



On 10/16/18 7:33 PM, David Holmes wrote:

Hi Dean,

Thanks for tackling this.

I'm still struggling to fully grasp why we need both the 
PerfCounters and the regular counters. I get that we have to 
decrement the live counts before ensure_join() has allowed 
Thread.join() to return, to ensure that if we then check the 
number of threads it has dropped by one. But I don't understand 
why that means we need to manage the thread count in two parts. 
Particularly as now you don't use the PerfCounter to return the 
live count, so it makes me wonder what role the PerfCounter is 
playing as it is temporarily inconsistent with the reported live 
count? 


Perf counters were added long time back in JDK 1.4.2 for 
performance measurement before java.lang.management API. One can 
use jstat tool to monitor VM perf counters of a running VM.   One 
could look into the possibility of deprecating these counters and 
remove them over time.



On 17/10/2018 9:43 AM, dean.l...@oracle.com wrote:
New webrev:

http://cr.openjdk.java.net/~dlong/8021335/webrev.4/


When the perf counters are updated when a thread is added/removed, 
it's holding Threads_lock.  Are the asserts in 
ThreadService::remove_thread necessary?




Not really.  They were intended to catch the case where the atomic 
counters weren't decremented for some reason, not for the perf 
counters.

Should I remove them?



Hmm... when remove_thread is called but decrement_thread_counts has 
not been called.   It's a bug in thread accounting.  It happens to 
have the perf counters that can be compared to assert. It seems not 
obvious.  Setting the perf counters same values as 
_atomic_threads_count and _atomic_daemon_threads_count makes sense 
to me.


I would opt for removing the asserts but I can't think of an 
alternative how to catch the issue you concern about.


For clarify, I think we could simply set _live_threads_count to 
the value of _atomic_threads_count and set _daemon_threads_count 
to the value of _atomic_daemon_threads_count.




I think that works, even inside decrement_thread_counts() without 
holding the Threads_lock.  If you agree, I'll make that change.



+1



New webrevs, full and incremental:

http://cr.openjdk.java.net/~dlong/8021335/webrev.6/
http://cr.openjdk.java.net/~dlong/8021335/webrev.6.diff/

I like it better without all the asserts too.

dl


Mandy

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

Thanks, Mandy!

dl

On 10/17/18 6:35 PM, Mandy Chung wrote:



On 10/17/18 3:18 PM, dean.l...@oracle.com wrote:


New webrevs, full and incremental:

http://cr.openjdk.java.net/~dlong/8021335/webrev.6/
http://cr.openjdk.java.net/~dlong/8021335/webrev.6.diff/

I like it better without all the asserts too.


Looks good to me.

Mandy

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

Thanks JC!

dl

On 10/17/18 5:06 PM, JC Beyler wrote:

Hi Dean,

The new webrev looks much better :) LGTM (not a reviewer though :-)).

Thanks,
Jc
On Wed, Oct 17, 2018 at 3:19 PM > wrote:


On 10/17/18 2:38 PM, Mandy Chung wrote:



On 10/17/18 2:13 PM, dean.l...@oracle.com
 wrote:

On 10/17/18 1:41 PM, Mandy Chung wrote:



On 10/16/18 7:33 PM, David Holmes wrote:

Hi Dean,

Thanks for tackling this.

I'm still struggling to fully grasp why we need both the
PerfCounters and the regular counters. I get that we have to
decrement the live counts before ensure_join() has allowed
Thread.join() to return, to ensure that if we then check the
number of threads it has dropped by one. But I don't
understand why that means we need to manage the thread count
in two parts. Particularly as now you don't use the
PerfCounter to return the live count, so it makes me wonder
what role the PerfCounter is playing as it is temporarily
inconsistent with the reported live count? 


Perf counters were added long time back in JDK 1.4.2 for
performance measurement before java.lang.management API.  One
can use jstat tool to monitor VM perf counters of a running
VM.   One could look into the possibility of deprecating these
counters and remove them over time.


On 17/10/2018 9:43 AM, dean.l...@oracle.com
 wrote:
New webrev:

http://cr.openjdk.java.net/~dlong/8021335/webrev.4/



When the perf counters are updated when a thread is
added/removed, it's holding Threads_lock.  Are the asserts in
ThreadService::remove_thread necessary?



Not really.  They were intended to catch the case where the
atomic counters weren't decremented for some reason, not for the
perf counters.
Should I remove them?



Hmm... when remove_thread is called but decrement_thread_counts
has not been called.   It's a bug in thread accounting.  It
happens to have the perf counters that can be compared to
assert.  It seems not obvious.  Setting the perf counters same
values as _atomic_threads_count and _atomic_daemon_threads_count
makes sense to me.

I would opt for removing the asserts but I can't think of an
alternative how to catch the issue you concern about.


For clarify, I think we could simply set _live_threads_count to
the value of _atomic_threads_count and set
_daemon_threads_count to the value of _atomic_daemon_threads_count.



I think that works, even inside decrement_thread_counts()
without holding the Threads_lock.  If you agree, I'll make that
change.


+1



New webrevs, full and incremental:

http://cr.openjdk.java.net/~dlong/8021335/webrev.6/

http://cr.openjdk.java.net/~dlong/8021335/webrev.6.diff/


I like it better without all the asserts too.

dl


Mandy




--

Thanks,
Jc

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/17/18 2:38 PM, Mandy Chung wrote:



On 10/17/18 2:13 PM, dean.l...@oracle.com wrote:

On 10/17/18 1:41 PM, Mandy Chung wrote:



On 10/16/18 7:33 PM, David Holmes wrote:

Hi Dean,

Thanks for tackling this.

I'm still struggling to fully grasp why we need both the 
PerfCounters and the regular counters. I get that we have to 
decrement the live counts before ensure_join() has allowed 
Thread.join() to return, to ensure that if we then check the number 
of threads it has dropped by one. But I don't understand why that 
means we need to manage the thread count in two parts. Particularly 
as now you don't use the PerfCounter to return the live count, so 
it makes me wonder what role the PerfCounter is playing as it is 
temporarily inconsistent with the reported live count? 


Perf counters were added long time back in JDK 1.4.2 for performance 
measurement before java.lang.management API.  One can use jstat tool 
to monitor VM perf counters of a running VM.   One could look into 
the possibility of deprecating these counters and remove them over time.



On 17/10/2018 9:43 AM, dean.l...@oracle.com wrote:
New webrev:

http://cr.openjdk.java.net/~dlong/8021335/webrev.4/


When the perf counters are updated when a thread is added/removed, 
it's holding Threads_lock.  Are the asserts in 
ThreadService::remove_thread necessary?




Not really.  They were intended to catch the case where the atomic 
counters weren't decremented for some reason, not for the perf counters.

Should I remove them?



Hmm... when remove_thread is called but decrement_thread_counts has 
not been called.   It's a bug in thread accounting.  It happens to 
have the perf counters that can be compared to assert. It seems not 
obvious.  Setting the perf counters same values as 
_atomic_threads_count and _atomic_daemon_threads_count makes sense to me.


I would opt for removing the asserts but I can't think of an 
alternative how to catch the issue you concern about.


For clarify, I think we could simply set _live_threads_count to the 
value of _atomic_threads_count and set _daemon_threads_count to the 
value of _atomic_daemon_threads_count.




I think that works, even inside decrement_thread_counts() without 
holding the Threads_lock.  If you agree, I'll make that change.



+1



New webrevs, full and incremental:

http://cr.openjdk.java.net/~dlong/8021335/webrev.6/
http://cr.openjdk.java.net/~dlong/8021335/webrev.6.diff/

I like it better without all the asserts too.

dl


Mandy

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/17/18 1:41 PM, Mandy Chung wrote:



On 10/16/18 7:33 PM, David Holmes wrote:

Hi Dean,

Thanks for tackling this.

I'm still struggling to fully grasp why we need both the PerfCounters 
and the regular counters. I get that we have to decrement the live 
counts before ensure_join() has allowed Thread.join() to return, to 
ensure that if we then check the number of threads it has dropped by 
one. But I don't understand why that means we need to manage the 
thread count in two parts. Particularly as now you don't use the 
PerfCounter to return the live count, so it makes me wonder what role 
the PerfCounter is playing as it is temporarily inconsistent with the 
reported live count? 


Perf counters were added long time back in JDK 1.4.2 for performance 
measurement before java.lang.management API.  One can use jstat tool 
to monitor VM perf counters of a running VM.   One could look into the 
possibility of deprecating these counters and remove them over time.



On 17/10/2018 9:43 AM, dean.l...@oracle.com wrote:
New webrev:

http://cr.openjdk.java.net/~dlong/8021335/webrev.4/


When the perf counters are updated when a thread is added/removed, 
it's holding Threads_lock.  Are the asserts in 
ThreadService::remove_thread necessary?




Not really.  They were intended to catch the case where the atomic 
counters weren't decremented for some reason, not for the perf counters.

Should I remove them?

For clarify, I think we could simply set _live_threads_count to the 
value of _atomic_threads_count and set _daemon_threads_count to the 
value of _atomic_daemon_threads_count.




I think that works, even inside decrement_thread_counts() without 
holding the Threads_lock.  If you agree, I'll make that change.


dl


Mandy

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/16/18 7:33 PM, David Holmes wrote:

Hi Dean,

Thanks for tackling this.

I'm still struggling to fully grasp why we need both the PerfCounters 
and the regular counters. I get that we have to decrement the live 
counts before ensure_join() has allowed Thread.join() to return, to 
ensure that if we then check the number of threads it has dropped by 
one. But I don't understand why that means we need to manage the 
thread count in two parts. Particularly as now you don't use the 
PerfCounter to return the live count, so it makes me wonder what role 
the PerfCounter is playing as it is temporarily inconsistent with the 
reported live count? Is it simply that we can't atomically decrement 
the PerfCounters, and we can't require the Threads_lock when 
decrementing?




Good questions.  I didn't know the history, so I tried not to change too 
much.  The PerfCounters appear to be there to support StatSampler.  I 
think it's OK for them to be a little out of sync. If we wanted to make 
them more in sync with the atomic counters, I think we could do any of 
the following:
- Grab Threads_lock before SR_lock() where we call 
current_thread_exiting, and update perf counts at that time
- instead of decrementing in remove_thread, do this in 
decrement_thread_counts

  _live_threads_count->set_value(_atomic_threads_count);
  _daemon_threads_count->set_value(_atomic_daemon_threads_count);
( I see that Mandy has suggested the same thing.)
- DO update the perf counts atomically
However, I consider the PerfCounters inconsistency a separate issue from 
this bug.


On the code itself one thing I find irksome is that we already have a 
daemon flag in remove_thread but decrement_thread_counts doesn't get 
passed that and so always re-examines the thread to see if it is a 
daemon. I know only one of the code paths has the daemon flag, so 
perhaps we should hoist the daemon check up into JavaThread::exit and 
then pass a daemon parameter to decrement_thread_counts.




OK, I've fixed this.

There's also some ambiguity (existing) in relation to the existence of 
the jt->threadObj() and its affect on whether the thread is considered 
a daemon or not. Your check has to mirror the existing checks - the 
threadObj may be NULL at removal if it was a failed attach, but the 
addition considers a JNI-attached thread to be non-daemon. Overall 
this seems buggy to me but as long as your check follows the same 
rules that is okay. In that regard JavaThread::exit should never 
encounter a NULL threadObj().




I refactored is_daemon checks into a single helper function.


Minor nit: I suggest moving the two occurrences of the comment

// Do not count VM internal or JVMTI agent threads

inside is_hidden_thread so that we don't have to remember to update 
the comments if the definition changes.




OK.  New webrev:

http://cr.openjdk.java.net/~dlong/8021335/webrev.5/

dl


Thanks,
David
-

On 17/10/2018 9:43 AM, dean.l...@oracle.com wrote:

New webrev:

http://cr.openjdk.java.net/~dlong/8021335/webrev.4/

dl

On 10/16/18 11:59 AM, dean.l...@oracle.com wrote:

On 10/16/18 10:28 AM, JC Beyler wrote:

Hi Dean,

I noticed a typo here :
"are atomically" is in the comment but should no longer be there I 
believe; the sentence probably should be:

// These 2 counters are like the above thread counts, but are



Fixed!

Any way we could move this test into a helper method and both 
add_thread/current_thread_exiting could use the same "ignore" 
thread code so that we ensure the two never get out of sync?


+  if (jt->is_hidden_from_external_view() ||
+      jt->is_jvmti_agent_thread()) {
+    return;
+  }



Good idea.  Fixed.

(Also by curiosity, add_thread has an assert on the Threads_lock 
but not thread_exiting?)




Right, I followed the existing pattern where 
current_thread_exiting() only uses the atomic counters, so it 
doesn't need Threads_lock.


dl


Thanks,
Jc


On Tue, Oct 16, 2018 at 9:52 AM > wrote:


    https://bugs.openjdk.java.net/browse/JDK-8021335
    http://cr.openjdk.java.net/~dlong/8021335/webrev.3/


    This change attempts to fix an old and intermittent problem with
    ThreadMXBean thread counts.
    Changes have 3 main parts:
    1. Make sure hidden threads don't affect counts (even when 
exiting)

    2. Fix race reading counts using atomic counters
    3. Remove some workarounds in test (because they hide bugs)

    dl



--

Thanks,
Jc

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

New webrev:

http://cr.openjdk.java.net/~dlong/8021335/webrev.4/

dl

On 10/16/18 11:59 AM, dean.l...@oracle.com wrote:

On 10/16/18 10:28 AM, JC Beyler wrote:

Hi Dean,

I noticed a typo here :
"are atomically" is in the comment but should no longer be there I 
believe; the sentence probably should be:

// These 2 counters are like the above thread counts, but are



Fixed!

Any way we could move this test into a helper method and both 
add_thread/current_thread_exiting could use the same "ignore" thread 
code so that we ensure the two never get out of sync?


+  if (jt->is_hidden_from_external_view() ||
+      jt->is_jvmti_agent_thread()) {
+    return;
+  }



Good idea.  Fixed.

(Also by curiosity, add_thread has an assert on the Threads_lock but 
not thread_exiting?)




Right, I followed the existing pattern where current_thread_exiting() 
only uses the atomic counters, so it doesn't need Threads_lock.


dl


Thanks,
Jc


On Tue, Oct 16, 2018 at 9:52 AM > wrote:


https://bugs.openjdk.java.net/browse/JDK-8021335
http://cr.openjdk.java.net/~dlong/8021335/webrev.3/


This change attempts to fix an old and intermittent problem with
ThreadMXBean thread counts.
Changes have 3 main parts:
1. Make sure hidden threads don't affect counts (even when exiting)
2. Fix race reading counts using atomic counters
3. Remove some workarounds in test (because they hide bugs)

dl



--

Thanks,
Jc

Re: RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

On 10/16/18 10:28 AM, JC Beyler wrote:

Hi Dean,

I noticed a typo here :
"are atomically" is in the comment but should no longer be there I 
believe; the sentence probably should be:

// These 2 counters are like the above thread counts, but are



Fixed!

Any way we could move this test into a helper method and both 
add_thread/current_thread_exiting could use the same "ignore" thread 
code so that we ensure the two never get out of sync?


+  if (jt->is_hidden_from_external_view() ||
+      jt->is_jvmti_agent_thread()) {
+    return;
+  }



Good idea.  Fixed.

(Also by curiosity, add_thread has an assert on the Threads_lock but 
not thread_exiting?)




Right, I followed the existing pattern where current_thread_exiting() 
only uses the atomic counters, so it doesn't need Threads_lock.


dl


Thanks,
Jc


On Tue, Oct 16, 2018 at 9:52 AM > wrote:


https://bugs.openjdk.java.net/browse/JDK-8021335
http://cr.openjdk.java.net/~dlong/8021335/webrev.3/


This change attempts to fix an old and intermittent problem with
ThreadMXBean thread counts.
Changes have 3 main parts:
1. Make sure hidden threads don't affect counts (even when exiting)
2. Fix race reading counts using atomic counters
3. Remove some workarounds in test (because they hide bugs)

dl



--

Thanks,
Jc

RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean . long]

https://bugs.openjdk.java.net/browse/JDK-8021335
http://cr.openjdk.java.net/~dlong/8021335/webrev.3/

This change attempts to fix an old and intermittent problem with 
ThreadMXBean thread counts.

Changes have 3 main parts:
1. Make sure hidden threads don't affect counts (even when exiting)
2. Fix race reading counts using atomic counters
3. Remove some workarounds in test (because they hide bugs)

dl

Re: RFR: (S) 8210512: [Testbug] vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails with unexpected size of ClassLoaderReference.referringObjects

[dean . long]

+1 for this change
+1 for learning new things :-)

dl

On 9/10/18 4:16 PM, serguei.spit...@oracle.com wrote:

I also tried to learn from your email exchange with Dean L.

Thanks,
Serguei


On 9/10/18 15:59, David Holmes wrote:

Thanks for the reviews Serguei and JC.

On 11/09/2018 8:10 AM, JC Beyler wrote:

Hi David,

Looks good to me (I'm not a reviewer but wanted to piggy-back and 
say I actually learnt quite a bit with the conversation on the 
original webrev).


:) I've learnt a few things with these changes too.

Cheers,
David


Thanks!
Jc

On Mon, Sep 10, 2018 at 2:59 PM serguei.spit...@oracle.com 
 > wrote:


    Hi David,

    It looks good to me.
    Thank you for taking care about it!

    Thanks,
    Serguei


    On 9/10/18 14:31, David Holmes wrote:
 > Bug: https://bugs.openjdk.java.net/browse/JDK-8210512
 > Webrev: http://cr.openjdk.java.net/~dholmes/8210512/webrev/

 >
 > After the fix for JDK-8209361 where we modified JVM-TI to 
treat an
 > unresolved CP klass entry to a loaded klass as a resolved CP 
entry,

 > the listed test starting failing due to finding an extra
    reference to
 > the test class. As outlined in the bug report this extra 
reference:

 >
 > 17: instance of java.lang.Class(reflected
 > class=nsk.share.jdi.TestClass1, id=792)
 >
 > actually comes from the class itself. Every classfile has a
 > self-referential entry in the constant pool (this_class in 
JVMS 4.1)

 > and that is what we were encountering here.
 >
 > For the empty class used in the test this reference remains
 > unresolved, but in general it could be resolved and would
    otherwise be
 > found. So the fix for the test is to now expect to always 
find this

 > self-reference.
 >
 > Thanks,
 > David



--

Thanks,
Jc

Re: RFR (S) 8210512: vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails with unexpected size of ClassLoaderReference.referringObjects

[dean . long]

On 9/10/18 1:40 AM, David Holmes wrote:

Hi Dean,

On 10/09/2018 5:31 PM, dean.l...@oracle.com wrote:
Hi David.  If a class references its own fields or methods, and that 
code is executed by the interpreter, then we should expect that 
constant pool entry to be resolved, shouldn't we?


Yes

Likewise the compiler will treat it as resolved.  If we treat is as 
unresolved, we run into the case where we executed compiled code for 
a method that would have resolved the constant pool entry, but JVMTI 
says we didn't.


I don't think the this_klass reference necessarily falls under that 
categorization though. In an empty class (as used in the test case) 
there's no bytecode that references any CP index for the current 
class. It's simply an artifact of the classfile format.



Isn't the test in error for assuming no eager resolution?


Hmmm. Yes I suppose that is right. If we did eager resolution then 
even the this_klass CP entry would have been resolved and the test 
would have then encountered it from day one.


What bothers me is the self-referential nature of this ... and the 
obscurity of it. I don't think anyone looking at the various heap 
reference API's would think about a reference-to-self.




Maybe for an empty class, but in general I would expect self references 
to be considered.


By the way, it looks like we always eagerly resolve this_class for 
unsafe-anonymous classes.


Can you point me at that code please?



http://hg.openjdk.java.net/jdk/jdk/file/4e99f412148f/src/hotspot/share/classfile/classFileParser.cpp#l5594

dl


Thanks,
David


dl

On 9/9/18 10:51 PM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8210512
Webrev: http://cr.openjdk.java.net/~dholmes/8210512/webrev/

After the fix for JDK-8209361 where we modified JVM-TI to treat an 
unresolved CP klass entry to a loaded klass as a resolved CP entry, 
the listed test starting failing due to finding an extra reference 
to the test class. As outlined in the bug report this extra reference:


17: instance of java.lang.Class(reflected 
class=nsk.share.jdi.TestClass1, id=792)


actually comes from the class itself. Every classfile has a 
self-referential entry in the constant pool (this_klass in JVMS 4.1) 
and that is what we were encountering here.


I would argue that such an unresolved reference from a class to 
itself should never be treated as a "real" reference from a JVM TI 
perspective, and so we ship skip it - which is what the fix does:


- if (klass == NULL) {
+ if (klass == NULL || klass == ik) {
   continue;

Testing: the test concerned
 joatc testing of test that failed in 8209361 (TBD)
 mach5 tiers 1-3 (TBD)
 jvmti (TBD)

As noted testing is still TBD other then actual test but there are 
technical delays with that so I'll get the RFR out anyway.


Thanks,
David

Re: RFR (S) 8210512: vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails with unexpected size of ClassLoaderReference.referringObjects

[dean . long]

Hi David.  If a class references its own fields or methods, and that 
code is executed by the interpreter, then we should expect that constant 
pool entry to be resolved, shouldn't we?  Likewise the compiler will 
treat it as resolved.  If we treat is as unresolved, we run into the 
case where we executed compiled code for a method that would have 
resolved the constant pool entry, but JVMTI says we didn't.


Isn't the test in error for assuming no eager resolution?

By the way, it looks like we always eagerly resolve this_class for 
unsafe-anonymous classes.


dl

On 9/9/18 10:51 PM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8210512
Webrev: http://cr.openjdk.java.net/~dholmes/8210512/webrev/

After the fix for JDK-8209361 where we modified JVM-TI to treat an 
unresolved CP klass entry to a loaded klass as a resolved CP entry, 
the listed test starting failing due to finding an extra reference to 
the test class. As outlined in the bug report this extra reference:


17: instance of java.lang.Class(reflected 
class=nsk.share.jdi.TestClass1, id=792)


actually comes from the class itself. Every classfile has a 
self-referential entry in the constant pool (this_klass in JVMS 4.1) 
and that is what we were encountering here.


I would argue that such an unresolved reference from a class to itself 
should never be treated as a "real" reference from a JVM TI 
perspective, and so we ship skip it - which is what the fix does:


- if (klass == NULL) {
+ if (klass == NULL || klass == ik) {
   continue;

Testing: the test concerned
 joatc testing of test that failed in 8209361 (TBD)
 mach5 tiers 1-3 (TBD)
 jvmti (TBD)

As noted testing is still TBD other then actual test but there are 
technical delays with that so I'll get the RFR out anyway.


Thanks,
David

Re: RFR(S): 8059334: nsk/jvmti/scenarios/hotswap/HS201/hs201t001 fails with exit code 0 after timeout

[dean . long]

OK thanks.

dl


On 11/6/17 10:56 AM, Chris Plummer wrote:

Hi Dean,

It looks like ciEnv::jvmti_state_changed() is used to support the 
JVMTI AddCapabilities() interface, which I believe typically a JVMTI 
agent uses to setup the available capabilities when the agent is first 
loaded (although capabilities can by changed afterwords also). So I 
don't see that code as being related to changing the thread to be 
changed to "interp only" mode.


thanks,

Chris

On 11/3/17 9:44 PM, dean.l...@oracle.com wrote:
I'm not an expert in this area of code, but I'm wondering about 
Vladimir's comment about ciEnv::jvmti_state_changed() in the bug 
report. With your fix, maybe we don't need to check 
ciEnv::jvmti_state_changed() (which doesn't seem to be enough by 
itself) and throw away the compiled result.  We could just keep it 
around so it can be used when "interp only" mode is switched off.


dl


On 11/3/17 5:25 PM, Chris Plummer wrote:

Hello,

Please review the following:

https://bugs.openjdk.java.net/browse/JDK-8059334
http://cr.openjdk.java.net/~cjplummer/8059334/webrev.00/webrev.open/

The CR is closed, so I'll try to explain the issue here. The very 
short explanation is that the JVMTI test was enabling SINGLE STEP 
and doing a PopFrame, but the test method managed to get compiled 
and started executing compiled after the thread was put in "interp 
only" mode (which should never happen) and before the PopFrame was 
processed. The cause is a lack of a check for "interp only" mode in 
the OSR related compilation policy code.


Details:

The test is testing JVMTI PopFrame support. The test thread has a 
small method that sits in a tight loop. It will never exit. The main 
thread enables SINGLE STEP on the test thread, and then does a 
PopFrame on the test thread to force it out of the looping method. 
When the test failed due to a time out, I noticed it was still stuck 
in the small method, even though a PopFrame had been requested. 
Further, I noticed the method was compiled, so there was no chance 
the method would ever detect that it should do a PopFrame. Since 
"interp only" mode for SINGLE STEP had been enabled, the method 
should not be running compiled, so clearly something went wrong that 
allowed it to compile and execute.


When SINGLE STEP is requested, JVMTI will deopt the topmost method 
(actually the top 2), put the thread in "interp only" mode, and then 
has checks to make sure the thread continues to execute interpreted. 
To avoid compilation when a back branch tries to trigger one, there 
is a check for "interp only" mode in SimpleThresholdPolicy::event(). 
If the thread is in "interp only" mode, it will prevent compilation. 
SimpleThresholdPolicy::event() is called (indirectly) by 
InterpreterRuntime::frequency_counter_overflow(), which is called 
from the interpreter when the back branch threshold is reached.


After some debugging I noticed when the test timeout happens, 
"interp only" mode is not yet enabled when 
InterpreterRuntime::frequency_counter_overflow() is called, but is 
enabled by the time InterpreterRuntime::frequency_counter_overflow() 
has done the lookup of the nm. So there is a race here allowing the 
thread to begin execution in a compiled method even though "interp 
only" mode is enabled. I think the reason is because we safepoint 
during the compilation, and this allows a SINGLE STEP request to be 
processed, which enables "interp only" mode.


I should add that initially I only saw this bug with -Xcomp, but 
eventually realized it was caused by disabling 
BackgroundCompilation. That makes it much more likely that a SINGLE 
STEP request will come in and be processed during the call to 
InterpreterRuntime::frequency_counter_overflow() (because it will 
block until the compilation completes).


I believe for the fix it is enough just to add an "interp only" mode 
check in InterpreterRuntime::frequency_counter_overflow() after the 
nm lookup, and set it nm to NULL if we are now in "interp only" 
mode. If we are not in "interp only" mode at this point (and start 
executing the compiled method) it should not be possible to enter 
"interp only" mode until we reach a safepoint at some later time, 
and at that point the method will be properly deopt so it can 
execute interpreted.


thanks,

Chris

Re: RFR(S): 8059334: nsk/jvmti/scenarios/hotswap/HS201/hs201t001 fails with exit code 0 after timeout

[dean . long]

I'm not an expert in this area of code, but I'm wondering about 
Vladimir's comment about ciEnv::jvmti_state_changed() in the bug 
report.  With your fix, maybe we don't need to check 
ciEnv::jvmti_state_changed() (which doesn't seem to be enough by itself) 
and throw away the compiled result.  We could just keep it around so it 
can be used when "interp only" mode is switched off.


dl


On 11/3/17 5:25 PM, Chris Plummer wrote:

Hello,

Please review the following:

https://bugs.openjdk.java.net/browse/JDK-8059334
http://cr.openjdk.java.net/~cjplummer/8059334/webrev.00/webrev.open/

The CR is closed, so I'll try to explain the issue here. The very 
short explanation is that the JVMTI test was enabling SINGLE STEP and 
doing a PopFrame, but the test method managed to get compiled and 
started executing compiled after the thread was put in "interp only" 
mode (which should never happen) and before the PopFrame was 
processed. The cause is a lack of a check for "interp only" mode in 
the OSR related compilation policy code.


Details:

The test is testing JVMTI PopFrame support. The test thread has a 
small method that sits in a tight loop. It will never exit. The main 
thread enables SINGLE STEP on the test thread, and then does a 
PopFrame on the test thread to force it out of the looping method. 
When the test failed due to a time out, I noticed it was still stuck 
in the small method, even though a PopFrame had been requested. 
Further, I noticed the method was compiled, so there was no chance the 
method would ever detect that it should do a PopFrame. Since "interp 
only" mode for SINGLE STEP had been enabled, the method should not be 
running compiled, so clearly something went wrong that allowed it to 
compile and execute.


When SINGLE STEP is requested, JVMTI will deopt the topmost method 
(actually the top 2), put the thread in "interp only" mode, and then 
has checks to make sure the thread continues to execute interpreted. 
To avoid compilation when a back branch tries to trigger one, there is 
a check for "interp only" mode in SimpleThresholdPolicy::event(). If 
the thread is in "interp only" mode, it will prevent compilation. 
SimpleThresholdPolicy::event() is called (indirectly) by 
InterpreterRuntime::frequency_counter_overflow(), which is called from 
the interpreter when the back branch threshold is reached.


After some debugging I noticed when the test timeout happens, "interp 
only" mode is not yet enabled when 
InterpreterRuntime::frequency_counter_overflow() is called, but is 
enabled by the time InterpreterRuntime::frequency_counter_overflow() 
has done the lookup of the nm. So there is a race here allowing the 
thread to begin execution in a compiled method even though "interp 
only" mode is enabled. I think the reason is because we safepoint 
during the compilation, and this allows a SINGLE STEP request to be 
processed, which enables "interp only" mode.


I should add that initially I only saw this bug with -Xcomp, but 
eventually realized it was caused by disabling BackgroundCompilation. 
That makes it much more likely that a SINGLE STEP request will come in 
and be processed during the call to 
InterpreterRuntime::frequency_counter_overflow() (because it will 
block until the compilation completes).


I believe for the fix it is enough just to add an "interp only" mode 
check in InterpreterRuntime::frequency_counter_overflow() after the nm 
lookup, and set it nm to NULL if we are now in "interp only" mode. If 
we are not in "interp only" mode at this point (and start executing 
the compiled method) it should not be possible to enter "interp only" 
mode until we reach a safepoint at some later time, and at that point 
the method will be properly deopt so it can execute interpreted.


thanks,

Chris

Re: RFR[9u-dev]: 8146683: check_addr0 should be more efficient

[Dean Long]

On 2/23/2016 10:07 AM, Daniel D. Daugherty wrote:

On 2/23/16 5:56 AM, Thomas Stüfe wrote:

Hi Cheleswer,


On Tue, Feb 23, 2016 at 9:38 AM, Cheleswer Sahu 


  wrote:


Hi,



Please review the code changes for
https://bugs.openjdk.java.net/browse/JDK-8146683 .



Webrev link: http://cr.openjdk.java.net/~csahu/8146683/

JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683



Bug Brief:

While investigating  bug 
https://bugs.openjdk.java.net/browse/JDK-8144483

it has been observed that "check_addr0() " function  is not written
efficiently.

This function is trying to read each "prmap_t" entry in 
"/proc/self/map"

file one by one which is time taking and can cause in long pauses.



Solution proposed:

Instead of reading each "prmap_t" entry in "/proc/self/map" file one by
one, read the entries in chunks. There can be many entries in "map" 
file,

so I have decided to read these

in chunks of 100  "prmap_t" entries. Reading entries in chunks saves 
a lot

of read calls and results in smaller pause times.





Regards,

Cheleswer



We saw cases, especially on Solaris, where the content of a file in the
proc file system changed under us while we were reading it. So this 
should

be kept in mind. The original code seems also broken in that regard,
because it assumes the ::read() call to read the full size of a prmap_t
entry, but it may theoretically read an incomplete entry.

As for your coding, you first estimate the size of the mapping file and
then use this to determine how many entries to read; but when 
reading, this
information may already be stale. I think it would be more robust and 
also

simpler to just try to read as many entries as possible - in chunks, to
make reading faster - until you get EOF.

Kind Regards, Thomas


I'm really sure that we've been down this road before. In particular,
Dmitry Samersoff has worked on this issue (or one very much like it)
before, but he is on vacation until the end of the month.



There was a similar issue with Linux /proc/self/maps, and whether it was 
safe to read the file with buffered stdio or not.  See 8009062 and 7017193.


dl


Dan




On Tue, Feb 23, 2016 at 9:38 AM, Cheleswer Sahu 


wrote:


Hi,



Please review the code changes for
https://bugs.openjdk.java.net/browse/JDK-8146683 .



Webrev link: http://cr.openjdk.java.net/~csahu/8146683/

JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683



Bug Brief:

While investigating  bug 
https://bugs.openjdk.java.net/browse/JDK-8144483

it has been observed that "check_addr0() " function  is not written
efficiently.

This function is trying to read each "prmap_t" entry in 
"/proc/self/map"

file one by one which is time taking and can cause in long pauses.



Solution proposed:

Instead of reading each "prmap_t" entry in "/proc/self/map" file one by
one, read the entries in chunks. There can be many entries in "map" 
file,

so I have decided to read these

in chunks of 100  "prmap_t" entries. Reading entries in chunks saves 
a lot

of read calls and results in smaller pause times.





Regards,

Cheleswer

Re: PING Re: RFR: 8078521: AARCH64: Add AArch64 SA support

[Dean Long]

I added hotspot-...@openjdk.java.net again.

It looks reasonable to me, but I'm not a Reviewer.

dl

On 5/6/2015 7:36 AM, Andrew Haley wrote:

On 04/29/2015 08:42 AM, Andrew Haley wrote:

http://cr.openjdk.java.net/~aph/8078521-2/

Any news on this?  It shouldn't be controversial at this point.

Thanks,
Andrew.

Re: RFR(S): 8073042: jcmd hangs until another jcmd is executed (which, in turn, also hangs) (on Windows)

[Dean Long]

Hi Markus.  OK, thanks for the explanation.

dl

On 3/3/2015 1:15 AM, Markus Gronlund wrote:

Hi Dean,

Thanks for your input.

I think a monitor would be less ideal here:

A monitor would introduce blocking in enqueuing operations.

The posters are (in this case) remotely injected threads (by another 
process), and the processes injecting those threads are in WaitForSingleObject() on those 
thread handles. If a monitor is used, it needs to be held by the AttachListener over 
executing (arbitrary) operations. Granted, each operation today is being executed in 
serial fashion (but it does not need to be), but I believe we would not like to have each 
client also block on the monitor as part of enqueuing their operation, just in order to 
issue the notify.

With a Windows Semaphore, clients can act asynchronously in regard to enqueuing. 
As long as the semaphore's current count is  0, there is no blocking on the 
part of the AttachListener thread, WaitForSingleObject() return immediately 
decrementing the current count.

Thanks
Markus


-Original Message-
From: Dean Long
Sent: den 3 mars 2015 02:32
To: Markus Gronlund; serviceability-dev@openjdk.java.net; 
hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(S): 8073042: jcmd hangs until another jcmd is executed (which, 
in turn, also hangs) (on Windows)

Couldn't you instead treat it like a monitor?  Then it's OK if only the first 
notify wakes up the blocked thread, as long as that thread only blocks when the 
queue is empty.  In other words, when it wakes up, it should process all the 
items in the queue before blocking again.

dl

On 3/2/2015 4:34 AM, Markus Gronlund wrote:

Greetings,

   


Kindly asking for reviews for the following changeset:

   


Bug: https://bugs.openjdk.java.net/browse/JDK-8073042

Webrev: http://cr.openjdk.java.net/~mgronlun/8073042/webrev01/

   


Description:

The signaling mechanism used to communicate about attaching operations under 
Windows currently only allows for a single outstanding item to be visible. This 
leads to issues, such as the one described in this bug, where clients assume 
their operations are under processing (they have been enqueued after all), but 
the AttachListener thread does not see, and hence do not process, these 
operations.

   


Analysis:

The _wakeup semaphore only allows for a single outstanding operation:

CreateSemaphore(NULL, 0, 1, NULL);

When a thread has enqueued an operation, it will notify the AttachListener 
thread through the semaphore, by:

::ReleaseSemaphore(wakeup(), 1, NULL); // this increases the semaphore
count by 1

This will signal the semaphore and wakeup the AttachListener thread which 
(most likely) is in WaitForSingleObject() for the semaphore to become signaled. When the 
semaphore is signaled and AttachListener returns from WaitForSingleObject(), the 
semaphore's count is decremented, and the semaphore again becomes non-signaled (goes from 
current count 1 (which is also maximum count) to zero).


Problem: multiple client threads will enqueue their operations if they manage 
to get the list mutex() - if they do, they will insert their op into the queue, 
and call:

::ReleaseSemaphore(wakeup(), 1, NULL);

This means we could have two (or more) client threads having posted operations, 
before the AttachListener thread becomes scheduled.

Since the semaphore created has a maximum count == 1, any subsequent calls to 
::ReleaseSemaphore(wakeup(), 1, NULL);, taking the the current count to  
maximum count, will have _no_effect - the current count of the semaphore remains 
at maximum count level AND ::ReleaseSemaphore(wakeup(), 1, NULL); returns FALSE - 
but currently there is no checking the return value here...
This means the client thread has managed to enqueue its op (and will move into 
ConnectPipe()), but the AttachListener will never see more than 1 enqueued op 
at any one time (hence it does not know it is expected to process another 
operation and signal the associated pipe).

This is how operations manage to get enqueued, but not processed until another 
thread eventually signals the semaphore by posting another op.

We must allow the semaphore to stay signaled when multiple ops are enqueued - 
and since we only allow preallocate_count number of operations to be enqueued, 
we can ensure the semaphore manages this upper limit as its maximum count.

   


Tests:

   


Jcmd/dcmd test

Attach tests

   


Thanks

Markus

Re: RFR(S): 8073042: jcmd hangs until another jcmd is executed (which, in turn, also hangs) (on Windows)

[Dean Long]

Couldn't you instead treat it like a monitor?  Then it's OK if only the 
first

notify wakes up the blocked thread, as long as that thread only blocks when
the queue is empty.  In other words, when it wakes up, it should process all
the items in the queue before blocking again.

dl

On 3/2/2015 4:34 AM, Markus Gronlund wrote:

Greetings,

  


Kindly asking for reviews for the following changeset:

  


Bug: https://bugs.openjdk.java.net/browse/JDK-8073042

Webrev: http://cr.openjdk.java.net/~mgronlun/8073042/webrev01/

  


Description:

The signaling mechanism used to communicate about attaching operations under 
Windows currently only allows for a single outstanding item to be visible. This 
leads to issues, such as the one described in this bug, where clients assume 
their operations are under processing (they have been enqueued after all), but 
the AttachListener thread does not see, and hence do not process, these 
operations.

  


Analysis:

The _wakeup semaphore only allows for a single outstanding operation:

CreateSemaphore(NULL, 0, 1, NULL);

When a thread has enqueued an operation, it will notify the AttachListener 
thread through the semaphore, by:

::ReleaseSemaphore(wakeup(), 1, NULL); // this increases the semaphore count by 
1

This will signal the semaphore and wakeup the AttachListener thread which 
(most likely) is in WaitForSingleObject() for the semaphore to become signaled. When the 
semaphore is signaled and AttachListener returns from WaitForSingleObject(), the 
semaphore's count is decremented, and the semaphore again becomes non-signaled (goes from 
current count 1 (which is also maximum count) to zero).


Problem: multiple client threads will enqueue their operations if they manage 
to get the list mutex() - if they do, they will insert their op into the queue, 
and call:

::ReleaseSemaphore(wakeup(), 1, NULL);

This means we could have two (or more) client threads having posted operations, 
before the AttachListener thread becomes scheduled.

Since the semaphore created has a maximum count == 1, any subsequent calls to 
::ReleaseSemaphore(wakeup(), 1, NULL);, taking the the current count to  
maximum count, will have _no_effect - the current count of the semaphore remains 
at maximum count level AND ::ReleaseSemaphore(wakeup(), 1, NULL); returns FALSE - 
but currently there is no checking the return value here...
This means the client thread has managed to enqueue its op (and will move into 
ConnectPipe()), but the AttachListener will never see more than 1 enqueued op 
at any one time (hence it does not know it is expected to process another 
operation and signal the associated pipe).

This is how operations manage to get enqueued, but not processed until another 
thread eventually signals the semaphore by posting another op.

We must allow the semaphore to stay signaled when multiple ops are enqueued - 
and since we only allow preallocate_count number of operations to be enqueued, 
we can ensure the semaphore manages this upper limit as its maximum count.

  


Tests:

  


Jcmd/dcmd test

Attach tests

  


Thanks

Markus

Re: Request for approval: Backport of 8069030

[Dean Long]

Ping.  Is there a different alias I should sent this to?

dl

On 2/9/2015 1:16 PM, Dean Long wrote:

I would like to backport 8069030 to 8u60:

  https://bugs.openjdk.java.net/browse/JDK-8069030
  http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/5960a65b0f54

The patch applied cleanly.

dl

Re: Request for approval: Backport of 8069030

[Dean Long]

Thanks David.

dl

On 2/11/2015 3:33 PM, David Holmes wrote:

I have no issue with this backport.

David

On 12/02/2015 7:00 AM, Dean Long wrote:

Ping.  Is there a different alias I should sent this to?

dl

On 2/9/2015 1:16 PM, Dean Long wrote:

I would like to backport 8069030 to 8u60:

  https://bugs.openjdk.java.net/browse/JDK-8069030
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/5960a65b0f54

The patch applied cleanly.

dl

Request for approval: Backport of 8069030

[Dean Long]

I would like to backport 8069030 to 8u60:

  https://bugs.openjdk.java.net/browse/JDK-8069030
  http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/5960a65b0f54

The patch applied cleanly.

dl

Re: RFR(XS) 8069030: support new PTRACE_GETREGSET

[Dean Long]

Thanks David and Staffan for the reviews.

dl

On 2/2/2015 5:07 PM, David Holmes wrote:

Looks good.

Thanks,
David

On 3/02/2015 8:46 AM, Dean Long wrote:

Ping.  I'm still waiting for a second review on this.

thanks,

dl

On 1/28/2015 5:15 PM, Dean Long wrote:

Adding serviceability-dev@openjdk.java.net.  Can I get another review
for this please?

thanks,

dl

On 1/26/2015 11:02 PM, Staffan Larsen wrote:

Looks good!

Thanks,
/Staffan


On 27 jan 2015, at 05:09, Dean Long dean.l...@oracle.com wrote:

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

http://cr.openjdk.java.net/~dlong/8069030/webrev.00/

Testing on linux x86 by temporarily undefining PTRACE_GETREGS_REQ so
that the new code
would get triggered, then stepping through jstack -m pid with
gdb to make sure the new
code executed correctly.

dl

Re: Review Request (S) 8025841: JVMTI: vtable stub dynamic code notification is misplaced

[Dean Long]

What's the cost for adding a new BufferBlob subtype?  We already have 
AdapterBlob and MethodHandlesAdapterBlob.


dl

On 2/6/2014 3:52 PM, Oleg Mazurov wrote:

My understanding was that a buffer blob was just that - a buffer. Could 
potentially contain code fragments of different kinds.
Thus, is_buffer_blob() was the closest type available. Agree that a dependency 
on its name is not reliable, though testing
will reveal if the condition turns false for vtable chunks due to a name 
change (I had to deal with that particular test, Serguei
should be able to identify it). Adding a comment to where the name is defined 
(vtableStubs.cpp) that such a dependency exists
is a good idea.
Thanks,

 -- Oleg

On Feb 6, 2014, at 3:32 PM, Coleen Phillimore wrote:


Hi, I clicked on this a couple times.  It seems okay but isn't there a safer way to identify code 
blobs that are vtable stubs, without looking at the name (which can change in while creating it).  
A comment at least when you create vtable chunks would be good.   It seems that someone 
might want to rename it vtable or itable buffers, or something like that.

thanks,
Coleen

On 2/6/14 6:17 PM, serguei.spit...@oracle.com wrote:

Runtime team,

This fix was reviewed by Vladimir K. and me.
Just wanted to make sure if you would like to review it as well.
If not, then I will push it.

Thanks,
Serguei

On 2/3/14 2:17 PM, serguei.spit...@oracle.com wrote:

Please, review the fix for:
   https://bugs.openjdk.java.net/browse/JDK-8025841


Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/omazurov/8025841-JVMTI-vtbl.1

Summary:

   The fix contributed by Oleg Mazurov to improve profiling data quality.
   It moves the vtable stub dynamic code notification to the right place.
   I've already reviewed the fix, and it looks good to me.

   Bug report description:

JVMTI_EVENT_DYNAMIC_CODE_GENERATED for vtable stub gets scheduled when
 a new chunk of memory for subsequent vtable and itable stubs is allocated.
 That chunk is uninitialized (contains zeros or garbage) although due to 
the fact
 that the actual event delivery is deferred, at least one vtable comes out 
right.

 This event should describe an individual vtable/itable stub (base address 
and size)
 and only after it's been created (memory is actually populated with code).
 Where VM diagnostic messages about vtable/itable stubs are issued upon
 -XX:+PrintAdapterHandlers appears exactly the right place for JVMTI events 
as well.

 Getting vtables/itables right is important in the context of performance 
analysis as
 that dynamically generated code may accumulate quite noticeable CPU time
 (especially itabes), sometimes larger than the actual Java methods called.


Testing:
   Oleg tested it in the Oracle Studio Performance Analyzer environment.
   nsk.jvmti, nsk.jdi, nsk.jdwp,
   In progress: Jtreg com/sun/jdi, java/lang/instrument


Thanks,
Serguei

Re: Review Request (S) 8025841: JVMTI: vtable stub dynamic code notification is misplaced

[Dean Long]

OK.  Your further improvement idea sounds promising.

dl

On 2/7/2014 3:55 PM, Oleg Mazurov wrote:
The cost might be minimal but that would be a move in the wrong 
direction in my opinion.
A larger problem is that BufferBlobs being just placeholders for 
dynamic code should not be
reported via JVMTI at all: when they are created they usually contain 
no executable code and
actual objects placed into such blobs are reported separately (that 
wasn't true for vtable/itable
stubs before this fix). That the same address, that of a BufferBlob 
and its first object, could be
reported twice is revealed by a comment to a loop that follows the 
problematic comparison:


src/share/vm/prims/jvmtiCodeBlobEvents.cpp:

 124   // exclude VtableStubs, which are processed separately
 125   if (cb-is_buffer_blob()  strcmp(cb-name(), vtable chunks) 
== 0) {

 126 return;
 127   }
 128
 129   // check if this starting address has been seen already - the
 130   // assumption is that stubs are inserted into the list before the
 131   // enclosing BufferBlobs.
 132   address addr = cb-code_begin();
 133   for (int i=0; i_global_code_blobs-length(); i++) {
 134 JvmtiCodeBlobDesc* scb = _global_code_blobs-at(i);
 135 if (addr == scb-code_begin()) {
 136   return;
 137 }
 138   }

I believe that now that the vtable stub problem is fixed the need for 
that check is gone
and both the strcmp() call and the following loop could be removed 
altogether, thus stopping
further processing for *any* BufferBlob and avoiding a O(n^2) overhead 
they were causing.
The scope of that change is much larger than the original problem 
entailed and would require
not just additional ad hoc testing on the JMTI consumer side but also 
thorough statical analysis

for all BufferBlob uses in the VM code.
In fact, I was going to file a linked JIRA issue on that further 
improvement and if my idea for

it holds true there would be no need for a new BufferBlob subtype.

-- Oleg

On 2/7/2014 1:06 PM, Dean Long wrote:
What's the cost for adding a new BufferBlob subtype?  We already have 
AdapterBlob and MethodHandlesAdapterBlob.


dl

On 2/6/2014 3:52 PM, Oleg Mazurov wrote:
My understanding was that a buffer blob was just that - a buffer. 
Could potentially contain code fragments of different kinds.
Thus, is_buffer_blob() was the closest type available. Agree that a 
dependency on its name is not reliable, though testing
will reveal if the condition turns false for vtable chunks due to 
a name change (I had to deal with that particular test, Serguei
should be able to identify it). Adding a comment to where the name 
is defined (vtableStubs.cpp) that such a dependency exists

is a good idea.
Thanks,

 -- Oleg

On Feb 6, 2014, at 3:32 PM, Coleen Phillimore wrote:

Hi, I clicked on this a couple times. It seems okay but isn't there 
a safer way to identify code blobs that are vtable stubs, without 
looking at the name (which can change in while creating it).  A 
comment at least when you create vtable chunks would be good.   
It seems that someone might want to rename it vtable or itable 
buffers, or something like that.


thanks,
Coleen

On 2/6/14 6:17 PM, serguei.spit...@oracle.com wrote:

Runtime team,

This fix was reviewed by Vladimir K. and me.
Just wanted to make sure if you would like to review it as well.
If not, then I will push it.

Thanks,
Serguei

On 2/3/14 2:17 PM, serguei.spit...@oracle.com wrote:

Please, review the fix for:
   https://bugs.openjdk.java.net/browse/JDK-8025841


Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/omazurov/8025841-JVMTI-vtbl.1 



Summary:

   The fix contributed by Oleg Mazurov to improve profiling data 
quality.
   It moves the vtable stub dynamic code notification to the 
right place.

   I've already reviewed the fix, and it looks good to me.

   Bug report description:

JVMTI_EVENT_DYNAMIC_CODE_GENERATED for vtable stub gets 
scheduled when
 a new chunk of memory for subsequent vtable and itable stubs 
is allocated.
 That chunk is uninitialized (contains zeros or garbage) 
although due to the fact
 that the actual event delivery is deferred, at least one 
vtable comes out right.


 This event should describe an individual vtable/itable stub 
(base address and size)
 and only after it's been created (memory is actually 
populated with code).
 Where VM diagnostic messages about vtable/itable stubs are 
issued upon
 -XX:+PrintAdapterHandlers appears exactly the right place 
for JVMTI events as well.


 Getting vtables/itables right is important in the context of 
performance analysis as
 that dynamically generated code may accumulate quite 
noticeable CPU time
 (especially itabes), sometimes larger than the actual Java 
methods called.



Testing:
   Oleg tested it in the Oracle Studio Performance Analyzer 
environment.

   nsk.jvmti, nsk.jdi, nsk.jdwp,
   In progress: Jtreg com/sun/jdi, java/lang/instrument


Thanks

Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror

[Dean Long]

It seems like you could take this opportunity to make these declared 
fields of java.lang.Class,
allowing, for example, getProtectionDomain0() to be a simple Java method 
instead of a native method.


dl

On 05/20/2013 03:39 PM, Coleen Phillimore wrote:
Summary: Inject protection_domain, signers, init_lock into 
java_lang_Class


Net footprint change is zero except that these fields are in Java heap 
rather than metaspace.  This helps a little with InstanceKlass size 
which is in fixed size space with UseCompressedKlassPointers. Included 
serviceability because there were SA changes to code that I don't know 
is used.


Future work is to remove the signers field and the unused 
SetProtectionDomain function.


open webrev at http://cr.openjdk.java.net/~coleenp/8003421/
bug link at http://bugs.sun.com/view_bug.do?bug_id=8003421

Tested with vm.quick.testlist, JPRT, jtreg java/security tests and 
jck8 tests.


Thanks,
Coleen

Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror

[Dean Long]

On 5/20/2013 7:50 PM, Coleen Phillimore wrote:

On 5/20/2013 8:42 PM, Dean Long wrote:
It seems like you could take this opportunity to make these declared 
fields of java.lang.Class,
allowing, for example, getProtectionDomain0() to be a simple Java 
method instead of a native method.


We actually use the protection domain and init_lock from within the 
vm, so we want to be able to see it.   Signers can be moved out 
eventually though.


The VM can already see Java fields.  You would just need to initialize 
_protection_domain_offset using compute_offset() like we do for other 
fields.  Java fields would also have the advantage of working with the 
new @Contended annotation, which probably doesn't work for injected 
fields.  However for backwards compatability with an older class library 
you could use compute_optional_offset() and inject the field only if 
it's missing.


dl


Thanks,
Coleen



dl

On 05/20/2013 03:39 PM, Coleen Phillimore wrote:
Summary: Inject protection_domain, signers, init_lock into 
java_lang_Class


Net footprint change is zero except that these fields are in Java 
heap rather than metaspace.  This helps a little with InstanceKlass 
size which is in fixed size space with UseCompressedKlassPointers. 
Included serviceability because there were SA changes to code that I 
don't know is used.


Future work is to remove the signers field and the unused 
SetProtectionDomain function.


open webrev at http://cr.openjdk.java.net/~coleenp/8003421/
bug link at http://bugs.sun.com/view_bug.do?bug_id=8003421

Tested with vm.quick.testlist, JPRT, jtreg java/security tests and 
jck8 tests.


Thanks,
Coleen

Re: RFR 8003421: NPG: Move oops out of InstanceKlass into mirror

[Dean Long]

Right, unless there is a way to inject them conditionally.

dl

On 5/20/2013 9:02 PM, Ioi Lam wrote:
But if you move these fields into Class.java (in JDK8), then hsx25 
will not run on JDK7 anymore, unless these fields are also added in 
Class.java in JDK7.


- Ioi

On 05/20/2013 05:42 PM, Dean Long wrote:
It seems like you could take this opportunity to make these declared 
fields of java.lang.Class,
allowing, for example, getProtectionDomain0() to be a simple Java 
method instead of a native method.


dl

On 05/20/2013 03:39 PM, Coleen Phillimore wrote:
Summary: Inject protection_domain, signers, init_lock into 
java_lang_Class


Net footprint change is zero except that these fields are in Java 
heap rather than metaspace.  This helps a little with InstanceKlass 
size which is in fixed size space with UseCompressedKlassPointers. 
Included serviceability because there were SA changes to code that I 
don't know is used.


Future work is to remove the signers field and the unused 
SetProtectionDomain function.


open webrev at http://cr.openjdk.java.net/~coleenp/8003421/
bug link at http://bugs.sun.com/view_bug.do?bug_id=8003421

Tested with vm.quick.testlist, JPRT, jtreg java/security tests and 
jck8 tests.


Thanks,
Coleen

Re: RFR(XS): 8009287 Uninitialised variable in hotspot/agent/src/os/linux/ps_core.c

[Dean Long]

What if allocate_init_map() fails to allocate memory?  Shouldn't 
add_class_share_map_info() follow the pattern of add_map_info():

return NULL or map, and have the caller check for NULL?

dl

On 3/4/2013 11:39 PM, David Holmes wrote:

Looks fine to me - thanks Staffan!

David

On 5/03/2013 5:24 PM, Staffan Larsen wrote:

A very small fix for a warning.

webrev: http://cr.openjdk.java.net/~sla/8009287/webrev.00/

Thanks,
/Staffan

Re: RFR(XS): 8009287 Uninitialised variable in hotspot/agent/src/os/linux/ps_core.c

[Dean Long]

On 3/5/2013 3:23 PM, David Holmes wrote:

On 6/03/2013 9:08 AM, Dean Long wrote:

What if allocate_init_map() fails to allocate memory?  Shouldn't
add_class_share_map_info() follow the pattern of add_map_info():
return NULL or map, and have the caller check for NULL?


AFAICS apart from one seeming bug, if we can't allocate a new map it 
does no harm in terms of the code that looks at the map. The bug is:


 177mp = ph-core-class_share_maps;
 178if (mp) {
 179   print_debug(can't locate map_info at 0x%lx, trying class 
share maps\n,

 180  addr);

where I think 178 should be if (mp==NULL). Everything else will just 
do nothing upon encountering a NULL map.


The fix addresses the parfait warning and leaves the code functionally 
unchanged. So any robustness issues would need to be done via a follow 
up RFE.


Why not just add return map at the end of the function?  It seems 
closer to the intent of the original code, assuming it

addresses the parfait warning.

dl


David


dl

On 3/4/2013 11:39 PM, David Holmes wrote:

Looks fine to me - thanks Staffan!

David

On 5/03/2013 5:24 PM, Staffan Larsen wrote:

A very small fix for a warning.

webrev: http://cr.openjdk.java.net/~sla/8009287/webrev.00/

Thanks,
/Staffan

Re: RFR(XS): 8009287 Uninitialised variable in hotspot/agent/src/os/linux/ps_core.c

[Dean Long]

On 3/5/2013 4:48 PM, David Holmes wrote:

On 6/03/2013 10:44 AM, Dean Long wrote:

On 3/5/2013 3:23 PM, David Holmes wrote:

On 6/03/2013 9:08 AM, Dean Long wrote:

What if allocate_init_map() fails to allocate memory?  Shouldn't
add_class_share_map_info() follow the pattern of add_map_info():
return NULL or map, and have the caller check for NULL?


AFAICS apart from one seeming bug, if we can't allocate a new map it
does no harm in terms of the code that looks at the map. The bug is:

 177mp = ph-core-class_share_maps;
 178if (mp) {
 179   print_debug(can't locate map_info at 0x%lx, trying class
share maps\n,
 180  addr);

where I think 178 should be if (mp==NULL). Everything else will just
do nothing upon encountering a NULL map.

The fix addresses the parfait warning and leaves the code functionally
unchanged. So any robustness issues would need to be done via a follow
up RFE.


Why not just add return map at the end of the function?  It seems
closer to the intent of the original code, assuming it
addresses the parfait warning.


Nothing expects a return value from that function so what point is 
there in returning something? I would expect that to simply produce 
another warning/error about a function return value being ignored.


If it complains about that then wouldn't we need to put (void) in front 
of all calls to strcat(), for example?
Anyway, I'm OK with the current change if Staffan files an RFE for other 
issue.


dl


David
-



dl


David


dl

On 3/4/2013 11:39 PM, David Holmes wrote:

Looks fine to me - thanks Staffan!

David

On 5/03/2013 5:24 PM, Staffan Larsen wrote:

A very small fix for a warning.

webrev: http://cr.openjdk.java.net/~sla/8009287/webrev.00/

Thanks,
/Staffan

Re: RFR(XXS): 8007147: Trace event ExecuteVMOperation may get dangling pointer

[Dean Long]

On 2/19/2013 11:00 AM, Staffan Larsen wrote:


On 19 feb 2013, at 19:56, Dean Long dean.l...@oracle.com 
mailto:dean.l...@oracle.com wrote:


When the VM_Operation is created, we could take a snapshot of the 
thread_id of the caller, then use that later.


One problem with that is if the thread_id gets reused for a new thread 
quickly after the old thread terminates. This is perhps ulikely, but 
could happen.


Or we could block the creating thread from fully exiting until the VM 
op executes.


That would introduce a lot more synchronization and state to keep 
track of.


I think we could do it with a reference count on the thread, a wait in 
thread exit, and a notify in the VM thread.
We have a similar dangling pointer problem with JVMTI (7154963), and a 
reference count should solve that

problem as well.

dl


/Staffan

Re: RFR: 7152800: All tests using the attach API fail with well-known file is not secure on Mac OS X

[Dean Long]

OK.

dl

On 3/20/2012 3:27 AM, Staffan Larsen wrote:

I couldn't make it work on OSX. But this means that we should probably chown 
the file explicitly on linux as well to make sure it gets the right permissions 
in the case where the directory has the setgid bit set.

Thanks,
/Staffan

On 19 mar 2012, at 20:38, Dean Long wrote:


On linux, you can control this behavior with the setgid mode bit on the parent 
directory.  Does that also work on OSX?

dl

On 3/19/2012 6:03 AM, Staffan Larsen wrote:

Please review the following fix:

Webrev: 
http://cr.openjdk.java.net/~sla/7152800/webrev.00/http://cr.openjdk.java.net/%7Esla/7152800/webrev.00/
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7152800

We sometimes get an error say well-known file is not secure when running 
attach-tests on mac osx. The reason is a difference in how files are created between 
linux and mac osx (and that the mac code in HotSpot originates from the linux code):

* On linux, the default group owner of a newly created file will be user's 
effective group (id -g).

* On macos, the default group owner of a newly created file will be group owner 
of the directory the file is in.

The attach framework will verify that the file has the same effective owner and 
group as the currently running process. This will be true on linux, since files 
are created with the effective user and group as owner. This will NOT be true 
always on macos, since the file can have a different group if the temporary 
directory has a different group than what we are currently running as.

The fix on macos is to change the group ownership of the well-known file to the 
effective group when the file is created.

The fix has been verified by manually changing the effective group of the 
launching user.

Thanks,
/Staffan

Re: RFR: 7152800: All tests using the attach API fail with well-known file is not secure on Mac OS X

[Dean Long]

On linux, you can control this behavior with the setgid mode bit on the 
parent directory.  Does that also work on OSX?


dl

On 3/19/2012 6:03 AM, Staffan Larsen wrote:

Please review the following fix:

Webrev: http://cr.openjdk.java.net/~sla/7152800/webrev.00/ 
http://cr.openjdk.java.net/%7Esla/7152800/webrev.00/

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7152800

We sometimes get an error say well-known file is not secure when 
running attach-tests on mac osx. The reason is a difference in how 
files are created between linux and mac osx (and that the mac code in 
HotSpot originates from the linux code):


* On linux, the default group owner of a newly created file will be 
user's effective group (id -g).


* On macos, the default group owner of a newly created file will be 
group owner of the directory the file is in.


The attach framework will verify that the file has the same effective 
owner and group as the currently running process. This will be true on 
linux, since files are created with the effective user and group as 
owner. This will NOT be true always on macos, since the file can have 
a different group if the temporary directory has a different group 
than what we are currently running as.


The fix on macos is to change the group ownership of the well-known 
file to the effective group when the file is created.


The fix has been verified by manually changing the effective group of 
the launching user.


Thanks,
/Staffan

Auto Reply: serviceability-dev Digest, Vol 48, Issue 5

[dean . long]

This is an auto-replied message. I am out of the office right now.