Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271461: CompileCommand support for hidden class methods #4926

Closed
wants to merge 14 commits into from

Conversation

DamonFool
Copy link
Member

@DamonFool DamonFool commented Jul 29, 2021

Hi all,

We'd like to improve the CompileCommand to support hidden class methods, which is helpful for debugging and perfmance analysis.

Current implementation of CompileCommand doesn't work for hidden class methods.
For example, if you run with java -Xcomp -Xbatch -XX:+PrintCompilation
The following hidden class methods were compiled:

   5317 1573    b  3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)
   5318 1574    b  4       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)
   6735 1938    b  3       java.util.ResourceBundle$ResourceBundleProviderHelper$$Lambda$2/0x0000000801041e80::run (12 bytes)
   6736 1939    b  4       java.util.ResourceBundle$ResourceBundleProviderHelper$$Lambda$2/0x0000000801041e80::run (12 bytes)
   7060 2029    b  3       java.util.ResourceBundle$ResourceBundleProviderHelper$$Lambda$58/0x800000060::run (8 bytes)
   7061 2030    b  4       java.util.ResourceBundle$ResourceBundleProviderHelper$$Lambda$58/0x800000060::run (8 bytes)
   7688 2153    b  3       java.util.ResourceBundle$ResourceBundleProviderHelper$$Lambda$4/0x0000000801043218::run (16 bytes)
   7688 2154    b  4       java.util.ResourceBundle$ResourceBundleProviderHelper$$Lambda$4/0x0000000801043218::run (16 bytes)

Then people may want to exclude the compilation of java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run like this

-XX:CompileCommand=exclude,'java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run'

But unfortunately, it doesn't work.

CompileCommand: An error occurred during parsing
Error: Method pattern uses '/' together with '::'
Line: 'exclude,java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run'

Usage: '-XX:CompileCommand=<option>,<method pattern>' - to set boolean option to true
Usage: '-XX:CompileCommand=<option>,<method pattern>,<value>'
Use:   '-XX:CompileCommand=help' for more information and to list all option.

The failing reason is that the name of java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run is actually java.util.ResourceBundle$$Lambda$1+0x00000008010413c8::run in the VM.
But "+" had been replaced with "/" when it was printed by PrintCompilation [1].

So for a hidden class method, "/" should be replaced with "+" to make CompileCommand work.

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/klass.cpp#L685


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8271461: CompileCommand support for hidden class methods

Reviewers

Contributors

  • Tianyelan <vhinf2047@gmail.com>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4926/head:pull/4926
$ git checkout pull/4926

Update a local copy of the PR:
$ git checkout pull/4926
$ git pull https://git.openjdk.java.net/jdk pull/4926/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4926

View PR using the GUI difftool:
$ git pr show -t 4926

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4926.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 29, 2021

👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@DamonFool The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-compiler label Jul 29, 2021
@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jul 29, 2021

/contributor add Tianyelan vhinf2047@gmail.com

@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@DamonFool
Contributor Tianyelan <vhinf2047@gmail.com> successfully added.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jul 29, 2021

/test

@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@DamonFool you need to get approval to run the tests in tier1 for commits up until 211ee2b

@openjdk openjdk bot added the test-request label Jul 29, 2021
@DamonFool DamonFool marked this pull request as ready for review Jul 29, 2021
@openjdk openjdk bot added the rfr label Jul 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 29, 2021

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jul 29, 2021

I don't believe this can actually work as the name of the hidden class may not be the same from run to run. The mangled name is formed from the address of the instanceKlass.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jul 30, 2021

I don't believe this can actually work as the name of the hidden class may not be the same from run to run. The mangled name is formed from the address of the instanceKlass.

Thanks @dholmes-ora for your review.

I don't think so.

For the jdk/incubator/vector/Int512VectorTests.java crash bug mentioned by @PaulSandoz [1], the crashed hidden class method is the same during my experiments (on my machine always jdk.incubator.vector.IntVector$$Lambda$327/0x00000008010cad20::apply).

Current CompileTask:
C2:  10672 1251    b        jdk.incubator.vector.IntVector$$Lambda$327/0x00000008010cad20::apply (12 bytes)

Stack: [0x000070000bf83000,0x000070000c083000],  sp=0x000070000c07e900,  free space=1006k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.dylib+0x11feda9]  VMError::report_and_die(int, char const*, char const*, __va_list_tag*, Thread*, unsigned char*, void*, void*, char const*, int, unsigned long)+0x6e9
V  [libjvm.dylib+0x11ff42b]  VMError::report_and_die(Thread*, void*, char const*, int, char const*, char const*, __va_list_tag*)+0x3b
V  [libjvm.dylib+0x5db6fd]  report_vm_error(char const*, int, char const*, char const*, ...)+0xdd
V  [libjvm.dylib+0xccb29]  vshiftI_varNode::emit(CodeBuffer&, PhaseRegAlloc*) const+0xb9
V  [libjvm.dylib+0xe40c72]  PhaseOutput::scratch_emit_size(Node const*)+0x302
V  [libjvm.dylib+0xe35ada]  PhaseOutput::shorten_branches(unsigned int*)+0x50a
V  [libjvm.dylib+0xe3521a]  PhaseOutput::Output()+0xc0a
V  [libjvm.dylib+0x581d41]  Compile::Code_Gen()+0x511
V  [libjvm.dylib+0x57ee07]  Compile::Compile(ciEnv*, ciMethod*, int, bool, bool, bool, bool, bool, DirectiveSet*)+0x1ac7
V  [libjvm.dylib+0x46e279]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x109
V  [libjvm.dylib+0x59bc7f]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0x93f
V  [libjvm.dylib+0x59b120]  CompileBroker::compiler_thread_loop()+0x2c0
V  [libjvm.dylib+0x11544e3]  JavaThread::thread_main_inner()+0x273
V  [libjvm.dylib+0x115423c]  JavaThread::run()+0x45c
V  [libjvm.dylib+0x1151277]  Thread::call_run()+0x177
V  [libjvm.dylib+0xe25660]  thread_native_entry(Thread*)+0x150
C  [libsystem_pthread.dylib+0x6109]  _pthread_start+0x94
C  [libsystem_pthread.dylib+0x1b8b]  thread_start+0xf

And for another performance analysis case, the inlined hidden class method is always the same during my experiments (on my machine always org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar$$Lambda$67/0x00000008010213a8::apply).

  11746  100             org.openjdk.bench.jdk.incubator.vector.operation.jmh_generated.ByteScalar_GT_jmhTest::GT_thrpt_jmhStub (53 bytes)
                            @ 7   java.lang.System::nanoTime (0 bytes)   (intrinsic)
                            @ 17   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar::GT (91 bytes)   force inline by CompileCommand
                              @ 8   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar$$Lambda$67/0x00000008010213a8::apply (9 bytes)   inline (hot)
                               \-> TypeProfile (179/179 counts) = org/openjdk/bench/jdk/incubator/vector/operation/ByteScalar$$Lambda$67+0x00000008010213a8
                                @ 5   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar::lambda$new$8 (5 bytes)   inline (hot)
                              @ 25   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar$$Lambda$68/0x00000008010215c8::apply (9 bytes)   inline (hot)
                               \-> TypeProfile (179/179 counts) = org/openjdk/bench/jdk/incubator/vector/operation/ByteScalar$$Lambda$68+0x00000008010215c8
                                @ 5   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar::lambda$new$9 (5 bytes)   inline (hot)
                              @ 66   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar::gt (11 bytes)   inline (hot)
                              @ 87   org.openjdk.jmh.infra.Blackhole::consume (28 bytes)   disallowed by CompileCommand
                            @ 17   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar::GT (91 bytes)   force inline by CompileCommand
                              @ 8   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar$$Lambda$67/0x00000008010213a8::apply (9 bytes)   inline (hot)
                               \-> TypeProfile (179/179 counts) = org/openjdk/bench/jdk/incubator/vector/operation/ByteScalar$$Lambda$67+0x00000008010213a8
                                @ 5   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar::lambda$new$8 (5 bytes)   inline (hot)
                              @ 25   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar$$Lambda$68/0x00000008010215c8::apply (9 bytes)   inline (hot)
                               \-> TypeProfile (179/179 counts) = org/openjdk/bench/jdk/incubator/vector/operation/ByteScalar$$Lambda$68+0x00000008010215c8
                                @ 5   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar::lambda$new$9 (5 bytes)   inline (hot)
                              @ 66   org.openjdk.bench.jdk.incubator.vector.operation.ByteScalar::gt (11 bytes)   inline (hot)
                              @ 87   org.openjdk.jmh.infra.Blackhole::consume (28 bytes)   disallowed by CompileCommand
                            @ 34   java.lang.System::nanoTime (0 bytes)   (intrinsic)

That's why I say it's helpful for debugging and performance analysis.
Thanks.

[1] #4924 (comment)

// So if "/" is followed with a digit or "*", it may be a hidden class method.
// There may be false positive cases, but all of them are harmless and won't make anything worse.
char next = *(lp + 1);
if (('0' <= next && next <= '9') || next == '*') {
Copy link
Member

@dholmes-ora dholmes-ora Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use isdigit(next)

Copy link
Member Author

@DamonFool DamonFool Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use isdigit(next)

Good suggestion.
Updated.
Thanks.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jul 30, 2021

In terms of simply expanding the capability of the command parser this seems reasonable - and that is all that the test verifies.

Whether it will actually work in terms of compiling the expected code is not something that is not guaranteed due to the hidden class name not being guaranteed to remain the same, but it may in practice occur enough to be useful.

Thanks,
David

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jul 30, 2021

In terms of simply expanding the capability of the command parser this seems reasonable - and that is all that the test verifies.

Whether it will actually work in terms of compiling the expected code is not something that is not guaranteed due to the hidden class name not being guaranteed to remain the same, but it may in practice occur enough to be useful.

Thanks,
David

Yes, I run 100 times with this script.

for i in {1..100}; do
  echo ${i} 
  `pwd`/build/linux-x86_64-server-fastdebug/images/jdk/bin/java \
    -Xcomp -Xbatch -XX:+PrintCompilation | tee fu-${i}.log
done

The hidden class name never changed.

$ grep "0x00000008010413c8::run" fu*.log | grep "made zombie"
fu-1.log:   8360 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-10.log:   8354 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-100.log:   8334 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-11.log:   8351 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-12.log:   8380 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-13.log:   8354 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-14.log:   8329 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-15.log:   8346 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-16.log:   8341 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-17.log:   8269 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-18.log:   8352 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-19.log:   8337 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-2.log:   8344 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-20.log:   8537 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-21.log:   8339 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-22.log:   8359 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-23.log:   8223 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-24.log:   8353 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-25.log:   8351 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-26.log:   8366 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-27.log:   8207 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-28.log:   8327 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-29.log:   8353 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-3.log:   8170 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-30.log:   8247 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-31.log:   8382 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-32.log:   8338 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-33.log:   8332 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-34.log:   8347 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-35.log:   8366 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-36.log:   8346 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-37.log:   8389 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-38.log:   8338 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-39.log:   8226 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-4.log:   8348 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-40.log:   8367 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-41.log:   8355 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-42.log:   8362 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-43.log:   8381 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-44.log:   8347 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-45.log:   8240 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-46.log:   8363 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-47.log:   8266 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-48.log:   8318 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-49.log:   8350 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-5.log:   8362 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-50.log:   8346 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-51.log:   8253 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-52.log:   8358 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-53.log:   8319 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-54.log:   8336 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-55.log:   8371 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-56.log:   8343 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-57.log:   8236 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-58.log:   8351 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-59.log:   8284 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-6.log:   8225 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-60.log:   8209 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-61.log:   8361 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-62.log:   8363 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-63.log:   8357 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-64.log:   8337 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-65.log:   8354 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-66.log:   8357 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-67.log:   8368 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-68.log:   8348 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-69.log:   8343 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-7.log:   8388 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-70.log:   8348 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-71.log:   8354 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-72.log:   8370 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-73.log:   8344 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-74.log:   8248 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-75.log:   8351 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-76.log:   8357 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-77.log:   8361 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-78.log:   8344 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-79.log:   8369 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-8.log:   8339 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-80.log:   8338 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-81.log:   8356 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-82.log:   8343 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-83.log:   8344 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-84.log:   8331 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-85.log:   8341 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-86.log:   8262 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-87.log:   8336 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-88.log:   8237 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-89.log:   8219 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-9.log:   8349 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-90.log:   8241 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-91.log:   8240 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-92.log:   8395 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-93.log:   8367 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-94.log:   8349 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-95.log:   8198 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-96.log:   8365 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-97.log:   8226 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-98.log:   8235 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie
fu-99.log:   8358 1583       3       java.util.ResourceBundle$$Lambda$1/0x00000008010413c8::run (8 bytes)   made zombie

@iklam
Copy link
Member

@iklam iklam commented Jul 30, 2021

There's no guarantee that the same class names will be used. Here's an example that prints out the name of the auto-generated Lambda proxy class which invokes the Lambda expression inside the test1() function:

$ java Lambda.java 1
Lambda$$Lambda$233/0x0000000800d4c1f0.run
$ java Lambda.java 2
Lambda$$Lambda$234/0x0000000800d4c3f8.run
import java.lang.StackWalker.StackFrame;
import java.util.List;
import java.util.stream.Collectors;
import java.util.Set;

public class Lambda {
    public static void main(String args[]) {
        if (args.length > 0 && args[0].equals("1")) {
            test1(); test0();
        } else {
            test0(); test1();
        }
    }

    static void test0() { doit(() -> {}); }
    static void test1() { doit(() -> { System.out.println(getCaller(1)); });  }
    static void doit(Runnable r) { r.run(); }

    // depth==0 returns the immediate caller of getCaller
    static String getCaller(int depth) {
        StackWalker walker = StackWalker.getInstance(
            Set.of(StackWalker.Option.RETAIN_CLASS_REFERENCE,
                   StackWalker.Option.SHOW_HIDDEN_FRAMES));
        List<StackFrame> stack = walker.walk(s -> s.limit(depth+2).collect(Collectors.toList()));
        StackFrame sf = stack.get(depth+1);
        return sf.getDeclaringClass().getName() + "." + sf.getMethodName();
    }
}

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jul 30, 2021

$ java Lambda.java 1

Thanks @iklam for your review.

I tested your example like this.

for i in {1..30}; do
  ${JDK}/bin/java \
    Lambda 1
done

====================================
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run
Lambda$$Lambda$1/0x0000000801000a00.run

And like this

for i in {1..30}; do
  ${JDK}/bin/java \
    Lambda 2
done

====================================
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run
Lambda$$Lambda$2/0x0000000801000c18.run

Once again, the hidden class name didn't change.

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Jul 30, 2021

At JDK level, the generated class name depends on a counter:(Test$$Lambda$215)

private static String lambdaClassName(Class<?> targetClass) {
String name = targetClass.getName();
if (targetClass.isHidden()) {
// use the original class name
name = name.replace('/', '_');
}
return name.replace('.', '/') + "$$Lambda$" + counter.incrementAndGet();
}

Later, class file parser mangles this class name:(Test$$Lambda$215+0x0000000801144200)

void ClassFileParser::mangle_hidden_class_name(InstanceKlass* const ik) {
ResourceMark rm;
// Construct hidden name from _class_name, "+", and &ik. Note that we can't
// use a '/' because that confuses finding the class's package. Also, can't
// use an illegal char such as ';' because that causes serialization issues
// and issues with hidden classes that create their own hidden classes.
char addr_buf[20];
if (DumpSharedSpaces) {
// We want stable names for the archived hidden classes (only for static
// archive for now). Spaces under default_SharedBaseAddress() will be
// occupied by the archive at run time, so we know that no dynamically
// loaded InstanceKlass will be placed under there.
static volatile size_t counter = 0;
Atomic::cmpxchg(&counter, (size_t)0, Arguments::default_SharedBaseAddress()); // initialize it
size_t new_id = Atomic::add(&counter, (size_t)1);
jio_snprintf(addr_buf, 20, SIZE_FORMAT_HEX, new_id);
} else {
jio_snprintf(addr_buf, 20, INTPTR_FORMAT, p2i(ik));
}
size_t new_name_len = _class_name->utf8_length() + 2 + strlen(addr_buf);
char* new_name = NEW_RESOURCE_ARRAY(char, new_name_len);
jio_snprintf(new_name, new_name_len, "%s+%s",
_class_name->as_C_string(), addr_buf);
update_class_name(SymbolTable::new_symbol(new_name));

When we want to print this class name, we will replace '+' with '/':(Test$$Lambda$215/0x0000000801144200)

// Replace the last '+' char with '/'.
static char* convert_hidden_name_to_java(Symbol* name) {
size_t name_len = name->utf8_length();
char* result = NEW_RESOURCE_ARRAY(char, name_len + 1);
name->as_klass_external_name(result, (int)name_len + 1);
for (int index = (int)name_len; index > 0; index--) {
if (result[index] == '+') {
result[index] = JVM_SIGNATURE_SLASH;
break;
}
}
return result;
}
// In product mode, this function doesn't have virtual function calls so
// there might be some performance advantage to handling InstanceKlass here.
const char* Klass::external_name() const {
if (is_instance_klass()) {
const InstanceKlass* ik = static_cast<const InstanceKlass*>(this);
if (ik->is_hidden()) {
char* result = convert_hidden_name_to_java(name());
return result;
}
} else if (is_objArray_klass() && ObjArrayKlass::cast(this)->bottom_klass()->is_hidden()) {
char* result = convert_hidden_name_to_java(name());
return result;
}
if (name() == NULL) return "<unknown>";
return name()->as_klass_external_name();
}

I don't fully practice what I see but it looks like the class name depends on class loading, there is no reliable way to ensure the same class loading order from run to run.

import java.util.ArrayList;
import java.util.HashMap;
import java.util.Random;

public class Test {
    static class Foo {
    }

    public static void main(String args[]) {
        for (int i = 0; i < 100000; i++) {
            test0();
            if (new Random().nextInt(50) > 25) {
                new Foo(); // class loading, -Xlog:class+load
            }
            test2();
        }
    }

    static void test0() {
        doit(() -> {
        });
    }

    static void test2() {
        doit(() -> {
        });
    }

    static void doit(Runnable r) {
        r.run();
    }
}
[9.716s][info][class,load] Test source: file:/home/qingfeng.yy/jdktip/mytest/Lambda.java
[9.724s][info][class,load] Test$$Lambda$216/0x0000000801144200 source: Test
[9.730s][info][class,load] java.util.random.RandomGenerator source: shared objects file
[9.732s][info][class,load] java.util.Random source: shared objects file
[9.745s][info][class,load] Test$Foo source: file:/home/qingfeng.yy/jdktip/mytest/Lambda.java
[9.751s][info][class,load] Test$$Lambda$217/0x0000000801144618 source: Test
[9.933s][info][class,load] Test source: file:/home/qingfeng.yy/jdktip/mytest/Lambda.java
[9.940s][info][class,load] Test$$Lambda$216/0x0000000801144200 source: Test
[9.946s][info][class,load] java.util.random.RandomGenerator source: shared objects file
[9.949s][info][class,load] java.util.Random source: shared objects file
[9.962s][info][class,load] Test$$Lambda$217/0x0000000801144418 source: Test
[9.968s][info][class,load] Test$Foo source: file:/home/qingfeng.yy/jdktip/mytest/Lambda.java

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jul 30, 2021

At JDK level, the generated class name depends on a counter:(Test$$Lambda$215)

private static String lambdaClassName(Class<?> targetClass) {
String name = targetClass.getName();
if (targetClass.isHidden()) {
// use the original class name
name = name.replace('/', '_');
}
return name.replace('.', '/') + "$$Lambda$" + counter.incrementAndGet();
}

Later, class file parser mangles this class name:(Test$$Lambda$215+0x0000000801144200)

void ClassFileParser::mangle_hidden_class_name(InstanceKlass* const ik) {
ResourceMark rm;
// Construct hidden name from _class_name, "+", and &ik. Note that we can't
// use a '/' because that confuses finding the class's package. Also, can't
// use an illegal char such as ';' because that causes serialization issues
// and issues with hidden classes that create their own hidden classes.
char addr_buf[20];
if (DumpSharedSpaces) {
// We want stable names for the archived hidden classes (only for static
// archive for now). Spaces under default_SharedBaseAddress() will be
// occupied by the archive at run time, so we know that no dynamically
// loaded InstanceKlass will be placed under there.
static volatile size_t counter = 0;
Atomic::cmpxchg(&counter, (size_t)0, Arguments::default_SharedBaseAddress()); // initialize it
size_t new_id = Atomic::add(&counter, (size_t)1);
jio_snprintf(addr_buf, 20, SIZE_FORMAT_HEX, new_id);
} else {
jio_snprintf(addr_buf, 20, INTPTR_FORMAT, p2i(ik));
}
size_t new_name_len = _class_name->utf8_length() + 2 + strlen(addr_buf);
char* new_name = NEW_RESOURCE_ARRAY(char, new_name_len);
jio_snprintf(new_name, new_name_len, "%s+%s",
_class_name->as_C_string(), addr_buf);
update_class_name(SymbolTable::new_symbol(new_name));

When we want to print this class name, we will replace '+' with '/':(Test$$Lambda$215/0x0000000801144200)

// Replace the last '+' char with '/'.
static char* convert_hidden_name_to_java(Symbol* name) {
size_t name_len = name->utf8_length();
char* result = NEW_RESOURCE_ARRAY(char, name_len + 1);
name->as_klass_external_name(result, (int)name_len + 1);
for (int index = (int)name_len; index > 0; index--) {
if (result[index] == '+') {
result[index] = JVM_SIGNATURE_SLASH;
break;
}
}
return result;
}
// In product mode, this function doesn't have virtual function calls so
// there might be some performance advantage to handling InstanceKlass here.
const char* Klass::external_name() const {
if (is_instance_klass()) {
const InstanceKlass* ik = static_cast<const InstanceKlass*>(this);
if (ik->is_hidden()) {
char* result = convert_hidden_name_to_java(name());
return result;
}
} else if (is_objArray_klass() && ObjArrayKlass::cast(this)->bottom_klass()->is_hidden()) {
char* result = convert_hidden_name_to_java(name());
return result;
}
if (name() == NULL) return "<unknown>";
return name()->as_klass_external_name();
}

I don't fully practice what I see but it looks like the class name depends on class loading, there is no reliable way to ensure the same class loading order from run to run.

import java.util.ArrayList;
import java.util.HashMap;
import java.util.Random;

public class Test {
    static class Foo {
    }

    public static void main(String args[]) {
        for (int i = 0; i < 100000; i++) {
            test0();
            if (new Random().nextInt(50) > 25) {
                new Foo(); // class loading, -Xlog:class+load
            }
            test2();
        }
    }

    static void test0() {
        doit(() -> {
        });
    }

    static void test2() {
        doit(() -> {
        });
    }

    static void doit(Runnable r) {
        r.run();
    }
}
[9.716s][info][class,load] Test source: file:/home/qingfeng.yy/jdktip/mytest/Lambda.java
[9.724s][info][class,load] Test$$Lambda$216/0x0000000801144200 source: Test
[9.730s][info][class,load] java.util.random.RandomGenerator source: shared objects file
[9.732s][info][class,load] java.util.Random source: shared objects file
[9.745s][info][class,load] Test$Foo source: file:/home/qingfeng.yy/jdktip/mytest/Lambda.java
[9.751s][info][class,load] Test$$Lambda$217/0x0000000801144618 source: Test
[9.933s][info][class,load] Test source: file:/home/qingfeng.yy/jdktip/mytest/Lambda.java
[9.940s][info][class,load] Test$$Lambda$216/0x0000000801144200 source: Test
[9.946s][info][class,load] java.util.random.RandomGenerator source: shared objects file
[9.949s][info][class,load] java.util.Random source: shared objects file
[9.962s][info][class,load] Test$$Lambda$217/0x0000000801144418 source: Test
[9.968s][info][class,load] Test$Foo source: file:/home/qingfeng.yy/jdktip/mytest/Lambda.java

I agree with you all that in theory the hidden class name may be different from run to run.
But the fact is that they actually don't change.
Even in your experiment above, right? @kelthuzadx

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Jul 30, 2021

Test$$Lambda$217/0x0000000801144618 is different with Test$$Lambda$217/0x0000000801144418

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jul 30, 2021

Test$$Lambda$217/0x0000000801144618 is different with Test$$Lambda$217/0x0000000801144418

Ah, you're right.
Maybe, we can use Test$$Lambda$217/*::test2 to handle this case.
In your example, the program behavior is different.

But for most debugging and performance analysis cases, the program behavior won't change.
So it's still helpful.

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Jul 30, 2021

Okay, Test$$Lambda$217/*::test2 works for this case.

As you said, it also works for most cases, so I'm fine with this change :-)

@dean-long
Copy link
Member

@dean-long dean-long commented Jul 30, 2021

According to the help output, package/Class.method() is the syntax, and package.Class::method() is for backwards compatibility. So I'm curious, does "exclude,java/util/ResourceBundle$$Lambda$1+0x00000008010413c8.run" already work?

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Jul 30, 2021

So I'm curious, does "exclude,java/util/ResourceBundle$$Lambda$1+0x00000008010413c8.run" already work?

Yes, it already works.
But I don't think people would like to use that format since PrintCompilation doesn't print that style.
Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 3, 2021

Have you looked at test/hotspot/jtreg/compiler/oracle/MethodMatcherTest.java? It's a test for just checking the matching using the whitebox API. It would be nice to have that extended with test cases for hidden class methods.

Good idea.
Updated.
Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 3, 2021

The /* pattern shouldn't be disable. It's just that it should only work for the class name, which means checking for the '.' separator.
OK: java.util.ResourceBundle$$Lambda$1/*::method
not OK: java.util/*.ResourceBundle$$Lambda$1/0x<digits>::method

The two test cases have been added.
Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 4, 2021

It still appears to apply to package names, not just class names.

is_hidden_calss_pattern will check whether '*' is followed by "::" so cases like java.util/*.ResourceBundle$$Lambda$1/0x<digits>::method won't be detected as hidden class pattern. @dean-long

@iklam
Copy link
Member

@iklam iklam commented Aug 4, 2021

It still appears to apply to package names, not just class names.

is_hidden_calss_pattern will check whether '*' is followed by "::" so cases like java.util/*.ResourceBundle$$Lambda$1/0x<digits>::method won't be detected as hidden class pattern. @dean-long

You also need this test case: java.util/*::method. It should be disallowed. Otherwise it will be transformed to java.util+*::method. (But I think you current code allows it).

I have the feeling that we are unnecessarily complicating the VM simply because PrintCompilation prints out /.

Why don't we change PrintCompilation to print out + instead?

It's not like anyone is depending of the current output of /, because it isn't accepted by -XX:CompileCommand anyway.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 4, 2021

It still appears to apply to package names, not just class names.

is_hidden_calss_pattern will check whether '*' is followed by "::" so cases like java.util/*.ResourceBundle$$Lambda$1/0x<digits>::method won't be detected as hidden class pattern. @dean-long

You also need this test case: java.util/*::method. It should be disallowed. Otherwise it will be transformed to java.util+*::method. (But I think you current code allows it).

I have the feeling that we are unnecessarily complicating the VM simply because PrintCompilation prints out /.

Yes, I think we've over engineered this issue.

I have no idea how to exclude cases like java.util/*::method while supporting cases like java.util.ResourceBundle$$Lambda$1/*::method.

But does it really matter if cases like java.util/*::method are disallowed or harmlessly false recognized as hidden class patterns?

Before this change, java.util/*::method is disallowed, so there is no effect to the compilers.
After this change, java.util/*::method would be allowed and replaced to java.util+*::method.
But since there are no methods matched with java.util+*::method, so there is actually no effect to the compilers.

That's why I said there may be false positive cases, but they are actually harmless at the very beginning.

If java.util/*::method must be disallowed for CompileCommand, then I would ask shall we also disallow java.util.*:::method?

My test shows that java.util.*:::method is actually allowed by CompileCommand.

java -XX:CompileCommand=exclude,'java.util.*:::method' -version
CompileCommand: exclude java/util/*.method bool exclude = true
openjdk version "18-internal" 2022-03-15
OpenJDK Runtime Environment (fastdebug build 18-internal+0-adhoc.fool.jdk)
OpenJDK 64-Bit Server VM (fastdebug build 18-internal+0-adhoc.fool.jdk, mixed mode)

Why don't we change PrintCompilation to print out + instead?

That's not what I want since both the java docs and your example say that there is a '/' in hidden class name.
Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 5, 2021

The /* pattern shouldn't be disable. It's just that it should only work for the class name, which means checking for the '.' separator.
OK: java.util.ResourceBundle$$Lambda$1/*::method
not OK: java.util/*.ResourceBundle$$Lambda$1/0x<digits>::method

Hi @dean-long ,

In order to disallow java.util/*::method mentioned by @iklam , I suggest to disallow java.util.ResourceBundle$$Lambda$1/*::method for hidden classes.

Yes , it won't work for java.util.ResourceBundle$$Lambda$1/*::method.
But we can still use java.util.ResourceBundle$$Lambda$1*::method or java.util.ResourceBundle$$Lambda$1/0*::method or java.util.ResourceBundle$$Lambda$1/0x*::method instead.

It's not perfect for hidden classes, but it's still better than no support for that style of hidden classes.
And it won't change the behavior of non-hidden classes in that way.

Or do you have any idea to disallow java.util/*::method while supporting java.util.ResourceBundle$$Lambda$1/*::method?

Thanks.

@dean-long
Copy link
Member

@dean-long dean-long commented Aug 5, 2021

Actually, I think java.util/*::method should be allowed, because the wildcard is in the last component. It's no different from mypkg.myclass/*::method, except that we usually think of "java.util" as a package name.
Are there any examples of hidden classes not having '$' in the class name? If not, then maybe that's an easy way to tighten up the hidden class checks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 5, 2021

Are there any examples of hidden classes not having '$' in the class name? If not, then maybe that's an easy way to tighten up the hidden class checks.

Sounds good.

I'm not 100% sure.
But we can simply add this rule for '/*' pattern to disallow java.util/*::method and support java.util.ResourceBundle$$Lambda$1/*::method
Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 5, 2021

You also need this test case: java.util/*::method.

Hi @iklam , this test has been added.

We are conservative for "/*::" pattern and will replace '/' with '+' only if '$' is present.
Now it works fine with your and @dean-long 's cases.
Thanks.

@iklam
Copy link
Member

@iklam iklam commented Aug 5, 2021

@dean-long: With abf4df5, we'll see some bizarre behavior. I think it will confuse users who are trying to use -XX:CompileCommand, as it's hard to explain why some of these work and others don't.

Given the method java.lang.Class::isArray():

  • java/*::isArray: no warning, does not match (it gets transformed to java+*.isArray)
  • java.*::isArray: matches
  • java/*.isArray: matches
  • java/l*::isArray: gives warning Error: Method pattern uses '/' together with '::', does not match
  • java/l*.isArray: matches

The latest version, 6bdcff2, seems just too complicated for what it tries to achieve. Also, we can have regular classes to have $ in their class names (e.g., Java inner classes), so the above problem still exists.

I think it's better to stick with the current JDK code and have a simple rule -- if you want to match hidden classes, use + in the class name.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 5, 2021

@dean-long: With abf4df5, we'll see some bizarre behavior. I think it will confuse users who are trying to use -XX:CompileCommand, as it's hard to explain why some of these work and others don't.

Given the method java.lang.Class::isArray():

  • java/*::isArray: no warning, does not match (it gets transformed to java+*.isArray)
  • java.*::isArray: matches
  • java/*.isArray: matches
  • java/l*::isArray: gives warning Error: Method pattern uses '/' together with '::', does not match
  • java/l*.isArray: matches

The latest version, 6bdcff2, seems just too complicated for what it tries to achieve. Also, we can have regular classes to have $ in their class names (e.g., Java inner classes), so the above problem still exists.

I think it's better to stick with the current JDK code and have a simple rule -- if you want to match hidden classes, use + in the class name.

Hi @iklam ,

There is no simple way to fix the problems you mentioned above.
And I don't want to improve the complexity either.

Not all people know the fact that '/' should be replaced by '+' for hidden classes.
And the java docs say there is a '/' in hidden class name.
So it's worthy supporting it directly.

To lower the complexity, there are still false positive cases.
And there seems no way to fix them perfectly.
But I don't think it really matters in practice since all of them are harmless.

There are also known bizarre false positive cases for CompileCommand, e.g,

java -XX:CompileCommand=exclude,'java.util.*:::method' -version
CompileCommand: exclude java/util/*.method bool exclude = true
openjdk version "18-internal" 2022-03-15
OpenJDK Runtime Environment (fastdebug build 18-internal+0-adhoc.fool.jdk)
OpenJDK 64-Bit Server VM (fastdebug build 18-internal+0-adhoc.fool.jdk, mixed mode)

But I don't think it's a problem in daily usage.

Let's see what @neliasso think of it.
If @neliasso , author of CompileCommand, also doesn't want to support it directly, I'll close this PR since too much time has been spent on this small enhancement.

Thanks.

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 6, 2021

I've tried my best to find a way to support hidden class methods directly, but it seems too hard to do so.
I agree with @iklam that java.util/*::method shouldn't be considered as a hidden class pattern since java.util is a well known package name.
And I also agree with @dean-long that java.util.ResourceBundle$$Lambda$1/*::method should be supported as hidden classes.
But I failed to find a solution to distinguish them precisely without too much complexity.

So I decide to withdraw the patch, which seems to be already complicated and still with false positive cases.
Thank you all, especially @iklam and @dean-long , for your review and discussion.

But I think we can still do something to make CompileCommand be more friendly with hidden classes.
I suggest to improve the error msg to print the tips that '/' should be replaced with '+' for hidden classes since not all people know the fact.
And patch has been updated.

Before:

CompileCommand: An error occurred during parsing
Error: Method pattern uses '/' together with '::'
Line: 'exclude,java.util.ResourceBundle$ResourceBundleProviderHelper$$Lambda$2/0x00000008010420e0::run'

After:

CompileCommand: An error occurred during parsing
Error: Method pattern uses '/' together with '::' (tips: replace '/' with '+' for hidden classes)
Line: 'exclude,java.util.ResourceBundle$ResourceBundleProviderHelper$$Lambda$2/0x00000008010420e0::run'

Testing:

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR
   jtreg:test/hotspot/jtreg/compiler/oracle              5     5     0     0
   jtreg:test/hotspot/jtreg/compiler/compilercontrol    37    37     0     0
==============================
TEST SUCCESS

What do you think?
Thanks.

@navyxliu
Copy link
Member

@navyxliu navyxliu commented Aug 6, 2021

hi, Jiefu,

I spent some time reading JEP 371: Hidden Classes. it describes a lot of "not discoverable". eg.

A hidden class is not anonymous. It has a name that is available via Class::getName and may be shown in diagnostics (such as the output of java -verbose:class), in JVM TI class loading events, in JFR events, and in stack traces. However, the name has a sufficiently unusual form that it effectively makes the class invisible to all other classes.

It seems to me that the hidden class purposely uses name mangling to reduce discoverability, hide from programmers. Unless the mangling scheme is well defined, I don't think we can have a maintainable pattern to match them all the time. Yes, regex or wildcard may work, but it needs extra cognitive load to master, doesn't it?

Copy link
Member

@navyxliu navyxliu left a comment

LGTM

iklam
iklam approved these changes Aug 6, 2021
Copy link
Member

@iklam iklam left a comment

LGTM. Just a small nit for the warning message.

I think we have struck a good balance between usability and maintainability. It's unfortunate that / was chosen as a separator. This makes it hard to decide whether a / in a pattern would match the package name or the class name.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 6, 2021

@DamonFool This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8271461: CompileCommand support for hidden class methods

Co-authored-by: Tianyelan <vhinf2047@gmail.com>
Reviewed-by: yyang, xliu, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 107 new commits pushed to the master branch:

  • fa36e33: 8271513: support JavaThreadIteratorWithHandle replacement by new ThreadsList::Iterator
  • cc61520: 8270842: G1: Only young regions need to redirty outside references in remset.
  • f4cf2f7: 8272095: ProblemList java/nio/channels/FileChannel/Transfer2GPlus.java on linux-aarch64
  • 0aca4f7: 8271868: Warn user when using mac-sign option with unsigned app-image.
  • b6a19f1: 8271128: InlineIntrinsics support for 32-bit ARM
  • c2b7fac: 8271896: Remove unnecessary top address checks in BOT
  • e7b6f48: 8265602: -XX:DumpLoadedClassList should support custom loaders
  • ea02dad: 8272067: Initial nroff manpage generation for JDK 18
  • adb0ae5: 8261441: JFR: Filename expansion
  • e38e365: 8271208: Typo in ModuleDescriptor.read javadoc
  • ... and 97 more: https://git.openjdk.java.net/jdk/compare/6afcf5f5a243be10e2ec61229819c298ccce3267...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Aug 6, 2021
@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Aug 6, 2021

Thanks @navyxliu and @iklam for your approval.
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Aug 6, 2021

Going to push as commit 38ff85c.
Since your change was applied there have been 109 commits pushed to the master branch:

  • c495ede: 8272099: mark hotspot runtime/Monitor tests which ignore external VM flags
  • e882087: 8271904: mark hotspot runtime/ClassFile tests which ignore external VM flags
  • fa36e33: 8271513: support JavaThreadIteratorWithHandle replacement by new ThreadsList::Iterator
  • cc61520: 8270842: G1: Only young regions need to redirty outside references in remset.
  • f4cf2f7: 8272095: ProblemList java/nio/channels/FileChannel/Transfer2GPlus.java on linux-aarch64
  • 0aca4f7: 8271868: Warn user when using mac-sign option with unsigned app-image.
  • b6a19f1: 8271128: InlineIntrinsics support for 32-bit ARM
  • c2b7fac: 8271896: Remove unnecessary top address checks in BOT
  • e7b6f48: 8265602: -XX:DumpLoadedClassList should support custom loaders
  • ea02dad: 8272067: Initial nroff manpage generation for JDK 18
  • ... and 99 more: https://git.openjdk.java.net/jdk/compare/6afcf5f5a243be10e2ec61229819c298ccce3267...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Aug 6, 2021
@openjdk openjdk bot added integrated and removed ready labels Aug 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 6, 2021

@DamonFool Pushed as commit 38ff85c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@openjdk openjdk bot removed the rfr label Aug 6, 2021
@DamonFool DamonFool deleted the JDK-8271461 branch Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler integrated test-request
7 participants