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

8266642: improve ResolvedMethodTable hash function #3901

Closed
wants to merge 10 commits into from

Conversation

D-D-H
Copy link
Contributor

@D-D-H D-D-H commented May 6, 2021

JDK-8249719 has fixed the bad hash function problem, however, the performance problem still exists when there are a large number of classes with the same name.
Adding the address of the corresponding ClassLoaderData as a factor of hash can solve the problem.


Progress

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

Issue

  • JDK-8266642: Improve ResolvedMethodTable hash function

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3901

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 6, 2021

👋 Welcome back ddong! 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 openjdk bot added the rfr label May 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 6, 2021

@D-D-H The following label will be automatically applied to this pull request:

  • hotspot

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 label May 6, 2021
@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented May 6, 2021

/label add hotspot-runtime

@openjdk openjdk bot added the hotspot-runtime label May 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 6, 2021

@D-D-H
The hotspot-runtime label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 6, 2021

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi,

Can you please elaborate on when there are large numbers of classes with the same name? I can see this may help that case but how likely is that case? And what is the impact on other cases as we're now increasing the overhead of the hash computation.

Thanks,
David

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented May 10, 2021

Hi David,

Thanks for your comment!

Can you please elaborate on when there are large numbers of classes with the same name? I can see this may help that case but how likely is that case?

Actually, the performance problem we noticed occurred in jdk 11u, here's a simplified piece of code:

import java.lang.invoke.*;
import java.util.*;

class Test {
    public static void main(String[] args) throws Throwable{
        MethodType mt = MethodType.methodType(void.class);

        List<MethodHandle> mhs = new ArrayList<>();
        for (int i = 0; i < 2000; i++) {
            // In practice, it will point to different methods
            mhs.add(MethodHandles.lookup().findVirtual(Test.class, "call",
                                                       MethodType.methodType(void.class))
                                 .bindTo(new Test()));
        }

        for (int i = 0; i < 128; i++) {
            for (MethodHandle mh : mhs) {
                mh.invokeExact();
            }
        }
    }

    void call() {}
}

Run: java -Xlog:membername+table=debug Test, and we can see that all of the LambdaForm$MH use a same slot.

[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800254840.invoke(Ljava/lang/Object;)V index 219
[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800254c40.invoke(Ljava/lang/Object;)V index 219
[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800255040.invoke(Ljava/lang/Object;)V index 219
[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800255440.invoke(Ljava/lang/Object;)V index 219

This problem doesn't exist in jdk upstream because of the name-mangling mechanism for hidden class(ClassFileParser::mangle_hidden_class_name).

However, if the user creates many classes with the same name by different classloaders, the performance problems will still appear, although I also think that the general user will not implement the code in this way.

My plan is to fix this problem in the upstream and then backport it to JDK 11, what do you think?

And what is the impact on other cases as we're now increasing the overhead of the hash computation.

I think the overhead is negligible, here is a benchmark based on ResolvedMethodTableHash.java:

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.*;
import org.openjdk.jmh.infra.Blackhole;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.List;
import java.util.Random;

@State(Scope.Benchmark)
public class MyBenchmark extends ClassLoader {

    // Produce a class file with the given name and a single method:
    //     public static native void m();
    private int writeClass(byte[] buf, String className) {
       ... same as ResolvedMethodTableHash.java
    }

    private List<Class<?>> classes = new ArrayList<>();
    private Random r = new Random();

    @Setup
    public void prepare() throws Exception {
        for (int i = 0; i < 5000; i ++) {
            byte[] buf = new byte[100];
            int size = writeClass(buf, "MH$" + i);
            classes.add(defineClass(null, buf, 0, size));
        }
    }


    @Benchmark
    @Fork(value=1)
    @Warmup(iterations = 5, time = 1)
    public void test(Blackhole bh) throws Exception {
        MethodHandle mh = MethodHandles.publicLookup().findStatic(classes.get(r.nextInt(5000)),
                                                                  "m", MethodType.methodType(void.class));
        if (mh == null) {
            throw new RuntimeException();
        }
    }
}

on linux x86_64,

result with the patch:

Result: 904126.215 ±(99.9%) 26603.356 ops/s [Average]
  Statistics: (min, avg, max) = (852079.715, 904126.215, 927791.630), stdev = 30636.464
  Confidence interval (99.9%): [877522.859, 930729.571]

Benchmark                Mode  Samples       Score  Score error  Units
o.s.MyBenchmark.test    thrpt       20  904126.215    26603.356  ops/s

result without the patch:

Result: 883669.014 ±(99.9%) 26455.778 ops/s [Average]
  Statistics: (min, avg, max) = (830830.551, 883669.014, 906978.220), stdev = 30466.514
  Confidence interval (99.9%): [857213.236, 910124.792]

Benchmark                Mode  Samples       Score  Score error  Units
o.s.MyBenchmark.test    thrpt       20  883669.014    26455.778  ops/s

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented May 11, 2021

Is there a real benchmark that demonstrates this, rather than a
micro-benchmark? I'm just trying to understand under conditions this is
a real problem.

Sorry, I don't have a real benchmark yet.
I think the code snippets I provided before will exist in many current applications that rely on method handles.

Is the output above from JDK 17 or JDK 11? It seems to show the mangled
(well "adorned") name, so shouldn't they map to different hash buckets
anyway?

From JDK 11.
The reason why the name displayed in the log is different is because the external name of the target klass is used (see Method::name_and_sig_as_C_string), and in 11, all java.lang.invoke.LambdaForm$MH have the same name( that's why the slot index are the same).

I'm not sure it is the right approach to "fix" a problem that doesn't
really exist in mainline just so it can be backported to 11u

From my perspective, I think that this problem still exists in the mainline, but users will hardly write code that causes this problem.

Since this patch will not cause obvious performance problems, I think it is harmless to add it to the main-line.

@iwanowww
Copy link

@iwanowww iwanowww commented May 12, 2021

The only way to trigger the problem without VM anonymous classes is to load the same class file by numerous class loaders and then construct MethodHandles for some method in that class. It should be affected by the same problem with the hash function which uses only symbolic info.

But I'm not sure how much numerous class loaders themselves will stretch the JVM. It can easily make the effects of ResolvedMethodTable negligible.

Nevertheless, IMO it still makes sense to improve the hash function.

unsigned int method_hash(const Method* method) {
  unsigned int hash = method->klass_name()->identity_hash();
  hash = (hash * 31) ^ method->name()->identity_hash();
  hash = (hash * 31) ^ method->signature()->identity_hash();
  return hash;
}

There's the method address available (Method*) which can easily be mixed (as an additional component or as a replacement for any of existing components: method name/signature/holder holder name.

For example, here's how Symbol::identity_hash() looks loke:

class Symbol : public MetaspaceObj {
...
  unsigned identity_hash() const {
    unsigned addr_bits = (unsigned)((uintptr_t)this >> (LogMinObjAlignmentInBytes + 3));
    return ((unsigned)extract_hash(_hash_and_refcount) & 0xffff) |
           ((addr_bits ^ (length() << 8) ^ (( _body[0] << 8) | _body[1])) << 16);
  }

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented May 12, 2021

The only way to trigger the problem without VM anonymous classes is to load the same class file by numerous class loaders and then construct MethodHandles for some method in that class. It should be affected by the same problem with the hash function which uses only symbolic info.

But I'm not sure how much numerous class loaders themselves will stretch the JVM. It can easily make the effects of ResolvedMethodTable negligible.

Nevertheless, IMO it still makes sense to improve the hash function.

unsigned int method_hash(const Method* method) {
  unsigned int hash = method->klass_name()->identity_hash();
  hash = (hash * 31) ^ method->name()->identity_hash();
  hash = (hash * 31) ^ method->signature()->identity_hash();
  return hash;
}

There's the method address available (Method*) which can easily be mixed (as an additional component or as a replacement for any of existing components: method name/signature/holder holder name.

For example, here's how Symbol::identity_hash() looks loke:

class Symbol : public MetaspaceObj {
...
  unsigned identity_hash() const {
    unsigned addr_bits = (unsigned)((uintptr_t)this >> (LogMinObjAlignmentInBytes + 3));
    return ((unsigned)extract_hash(_hash_and_refcount) & 0xffff) |
           ((addr_bits ^ (length() << 8) ^ (( _body[0] << 8) | _body[1])) << 16);
  }

Hi Vladimir,

Thanks for the comment.
I have updated the implementation according to your suggestion.

Thanks,
Denghui

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented May 13, 2021

test with this improvement:

Passed: runtime/MemberName/ResolvedMethodTableHash.java
  build: 0.587 seconds
  compile: 0.585 seconds
  main: 3.495 seconds
Test results: passed: 1

test without this improvement:

Passed: runtime/MemberName/ResolvedMethodTableHash.java
  build: 0.581 seconds
  compile: 0.579 seconds
  main: 10.896 seconds
Test results: passed: 1

@iwanowww
Copy link

@iwanowww iwanowww commented May 14, 2021

What about the following variant?

diff --git a/src/hotspot/share/classfile/classLoaderData.hpp b/src/hotspot/share/classfile/classLoaderData.hpp
index 6fd5b9b8203..f1f1c82f591 100644
--- a/src/hotspot/share/classfile/classLoaderData.hpp
+++ b/src/hotspot/share/classfile/classLoaderData.hpp
@@ -325,6 +325,11 @@ class ClassLoaderData : public CHeapObj<mtClass> {
   const char* loader_name_and_id() const;
   Symbol* name_and_id() const { return _name_and_id; }
 
+  unsigned identity_hash() const {
+    unsigned addr_bits = (unsigned)((uintptr_t)this >> (LogMinObjAlignmentInBytes + 3));
+    return addr_bits;
+  }
+
   JFR_ONLY(DEFINE_TRACE_ID_METHODS;)
 };
 
diff --git a/src/hotspot/share/prims/resolvedMethodTable.cpp b/src/hotspot/share/prims/resolvedMethodTable.cpp
index 5e5761094f1..527a39f3646 100644
--- a/src/hotspot/share/prims/resolvedMethodTable.cpp
+++ b/src/hotspot/share/prims/resolvedMethodTable.cpp
@@ -23,6 +23,7 @@
  */
 
 #include "precompiled.hpp"
+#include "classfile/classLoaderData.hpp"
 #include "classfile/javaClasses.hpp"
 #include "gc/shared/oopStorage.inline.hpp"
 #include "gc/shared/oopStorageSet.hpp"
@@ -53,7 +54,8 @@ static const size_t GROW_HINT = 32;
 static const size_t ResolvedMethodTableSizeLog = 10;
 
 unsigned int method_hash(const Method* method) {
-  unsigned int hash = method->klass_name()->identity_hash();
+  unsigned int hash = method->method_holder()->class_loader_data()->identity_hash();
+  hash = (hash * 31) ^ method->klass_name()->identity_hash();
   hash = (hash * 31) ^ method->name()->identity_hash();
   hash = (hash * 31) ^ method->signature()->identity_hash();
   return hash;

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented May 14, 2021

@iwanowww
It's okay for me, updated.

Thanks

Copy link

@iwanowww iwanowww left a comment

Looks good to me, but, please, wait for somebody from Runtime team to approve it.

@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2021

@D-D-H 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:

8266642: improve ResolvedMethodTable hash function

Reviewed-by: vlivanov, coleenp

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 195 new commits pushed to the master branch:

  • fdd0352: 8267338: [JVMCI] revive JVMCI API removed by JDK-8243287
  • 0b49f5a: 8267257: Shenandoah: Always deduplicate strings when it is enabled during full gc
  • 12050f0: 8266651: Convert Table method parameters from String to Content
  • e749f75: 8267304: Bump global JTReg memory limit to 768m
  • e858dd6: 8267361: JavaTokenizer reads octal numbers mistakenly
  • 1b93b81: 8267133: jdk/jfr/event/gc/collection/TestG1ParallelPhases.java fails with Not expected phases: RestorePreservedMarks, RemoveSelfForwardingPtr: expected true, was false
  • 88b1142: 8267357: build breaks with -Werror option on micro benchmark added for JDK-8256973
  • 6ef46ce: 8231672: Simplify the reference processing parallelization framework
  • 392f962: 8267151: C2: Don't create dummy Opaque1Node for outmost unswitched IfNode
  • 0cf7e57: 8267239: C1: RangeCheckElimination for % operator if divisor is IntConstant
  • ... and 185 more: https://git.openjdk.java.net/jdk/compare/20ad42897427483a4a62e1de7e78d4620eb9e240...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @iwanowww, @coleenp, @stefank) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label May 14, 2021
Copy link
Contributor

@coleenp coleenp left a comment

Looks good to me, with the inclusion of classLoaderData.hpp in resolvedMethodTable.hpp.

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented May 14, 2021

/integrate

@openjdk openjdk bot added the sponsor label May 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2021

@D-D-H
Your change (at version e9c674c) is now ready to be sponsored by a Committer.

Copy link

@iwanowww iwanowww left a comment

I'll submit the patch for testing and sponsor once it is over.

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-dev:

On 11/05/2021 3:43 am, Denghui Dong wrote:

On Fri, 7 May 2021 06:27:25 GMT, Denghui Dong <ddong at openjdk.org> wrote:

JDK-8249719 has fixed the bad hash function problem, however, the performance problem still exists when there are a large number of classes with the same name.
Adding the address of the corresponding ClassLoaderData as a factor of hash can solve the problem.

Denghui Dong has updated the pull request incrementally with one additional commit since the last revision:

minor adjustment

Hi David,

Thanks for your comment!

Can you please elaborate on when there are large numbers of classes with the same name? I can see this may help that case but how likely is that case?

Actually, the performance problem we noticed occurred in jdk 11u, here's a simplified piece of code:

Is there a real benchmark that demonstrates this, rather than a
micro-benchmark? I'm just trying to understand under conditions this is
a real problem.

import java.lang.invoke.*;
import java.util.*;

class Test {
public static void main(String[] args) throws Throwable{
MethodType mt = MethodType.methodType(void.class);

     List\<MethodHandle> mhs \= new ArrayList\<>\(\)\;
     for \(int i \= 0\; i \< 2000\; i\+\+\) \{
         \/\/ In practice\, it will point to different methods
         mhs\.add\(MethodHandles\.lookup\(\)\.findVirtual\(Test\.class\, \"call\"\,
                                                    MethodType\.methodType\(void\.class\)\)
                              \.bindTo\(new Test\(\)\)\)\;
     \}

     for \(int i \= 0\; i \< 128\; i\+\+\) \{
         for \(MethodHandle mh \: mhs\) \{
             mh\.invokeExact\(\)\;
         \}
     \}
 \}

 void call\(\) \{\}

}

Run: `java -Xlog:membername+table=debug Test`, and we can see that all of the LambdaForm$MH use a same slot.

[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800254840.invoke(Ljava/lang/Object;)V index 219
[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800254c40.invoke(Ljava/lang/Object;)V index 219
[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800255040.invoke(Ljava/lang/Object;)V index 219
[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800255440.invoke(Ljava/lang/Object;)V index 219

This problem doesn't exist in jdk upstream because of the name-mangling mechanism for hidden class(ClassFileParser::mangle_hidden_class_name).

Is the output above from JDK 17 or JDK 11? It seems to show the mangled
(well "adorned") name, so shouldn't they map to different hash buckets
anyway?

However, if the user creates many classes with the same name by different classloaders, the performance problems will still appear, although I also think that the general user will not implement the code in this way.

My plan is to fix this problem in the upstream and then backport it to JDK 11, what do you think?

I'm not sure it is the right approach to "fix" a problem that doesn't
really exist in mainline just so it can be backported to 11u

And what is the impact on other cases as we're now increasing the overhead of the hash computation.

I think the overhead is negligible, here is a benchmark based on ResolvedMethodTableHash.java:

Thanks for confirming.

David

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-dev:

On 11/05/2021 3:43 am, Denghui Dong wrote:

On Fri, 7 May 2021 06:27:25 GMT, Denghui Dong <ddong at openjdk.org> wrote:

JDK-8249719 has fixed the bad hash function problem, however, the performance problem still exists when there are a large number of classes with the same name.
Adding the address of the corresponding ClassLoaderData as a factor of hash can solve the problem.

Denghui Dong has updated the pull request incrementally with one additional commit since the last revision:

minor adjustment

Hi David,

Thanks for your comment!

Can you please elaborate on when there are large numbers of classes with the same name? I can see this may help that case but how likely is that case?

Actually, the performance problem we noticed occurred in jdk 11u, here's a simplified piece of code:

Is there a real benchmark that demonstrates this, rather than a
micro-benchmark? I'm just trying to understand under conditions this is
a real problem.

import java.lang.invoke.*;
import java.util.*;

class Test {
public static void main(String[] args) throws Throwable{
MethodType mt = MethodType.methodType(void.class);

     List\<MethodHandle> mhs \= new ArrayList\<>\(\)\;
     for \(int i \= 0\; i \< 2000\; i\+\+\) \{
         \/\/ In practice\, it will point to different methods
         mhs\.add\(MethodHandles\.lookup\(\)\.findVirtual\(Test\.class\, \"call\"\,
                                                    MethodType\.methodType\(void\.class\)\)
                              \.bindTo\(new Test\(\)\)\)\;
     \}

     for \(int i \= 0\; i \< 128\; i\+\+\) \{
         for \(MethodHandle mh \: mhs\) \{
             mh\.invokeExact\(\)\;
         \}
     \}
 \}

 void call\(\) \{\}

}

Run: `java -Xlog:membername+table=debug Test`, and we can see that all of the LambdaForm$MH use a same slot.

[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800254840.invoke(Ljava/lang/Object;)V index 219
[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800254c40.invoke(Ljava/lang/Object;)V index 219
[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800255040.invoke(Ljava/lang/Object;)V index 219
[0.359s][debug][membername,table] ResolvedMethod entry added for java.lang.invoke.LambdaForm$MH/0x0000000800255440.invoke(Ljava/lang/Object;)V index 219

This problem doesn't exist in jdk upstream because of the name-mangling mechanism for hidden class(ClassFileParser::mangle_hidden_class_name).

Is the output above from JDK 17 or JDK 11? It seems to show the mangled
(well "adorned") name, so shouldn't they map to different hash buckets
anyway?

However, if the user creates many classes with the same name by different classloaders, the performance problems will still appear, although I also think that the general user will not implement the code in this way.

My plan is to fix this problem in the upstream and then backport it to JDK 11, what do you think?

I'm not sure it is the right approach to "fix" a problem that doesn't
really exist in mainline just so it can be backported to 11u

And what is the impact on other cases as we're now increasing the overhead of the hash computation.

I think the overhead is negligible, here is a benchmark based on ResolvedMethodTableHash.java:

Thanks for confirming.

David

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-dev:

On 11/05/2021 12:37 pm, Denghui Dong wrote:

On Tue, 11 May 2021 00:27:47 GMT, David Holmes <dholmes at openjdk.org> wrote:

Because here needs different class loader data to load the different classes with the same name, I think Unsafe API is a simple way to achieve this purpose.

But as you note, the name mangling for hidden classes would mean that the names are not actually the same. And I don't see how this introduces a different classloader-data. ??

Unsafe.getUnsafe().defineAnonymousClass will create an unsafe anonymous class rather than a hidden class, and for unsafe anonymous class, JVM will create a new CLD, see the comment in SystemDictionary::resolve_hidden_class_from_stream:

// - for unsafe anonymous class: create a new CLD whith a class holder that uses
// the same class loader as the unsafe_anonymous_host.
// - for hidden classes that are not strong: create a new CLD that has a class holder and
// whose loader is the Lookup class's loader.
// - for hidden class: add the class to the Lookup class's loader's CLD.

Okay but we should only be concerned about the behaviour of public
API's, so we should be testing and benchmarking those API's.

defineAnonymousClass() maybe relevant for 11u, but not mainline.

Thanks,
David

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-dev:

On 11/05/2021 12:37 pm, Denghui Dong wrote:

On Tue, 11 May 2021 00:27:47 GMT, David Holmes <dholmes at openjdk.org> wrote:

Because here needs different class loader data to load the different classes with the same name, I think Unsafe API is a simple way to achieve this purpose.

But as you note, the name mangling for hidden classes would mean that the names are not actually the same. And I don't see how this introduces a different classloader-data. ??

Unsafe.getUnsafe().defineAnonymousClass will create an unsafe anonymous class rather than a hidden class, and for unsafe anonymous class, JVM will create a new CLD, see the comment in SystemDictionary::resolve_hidden_class_from_stream:

// - for unsafe anonymous class: create a new CLD whith a class holder that uses
// the same class loader as the unsafe_anonymous_host.
// - for hidden classes that are not strong: create a new CLD that has a class holder and
// whose loader is the Lookup class's loader.
// - for hidden class: add the class to the Lookup class's loader's CLD.

Okay but we should only be concerned about the behaviour of public
API's, so we should be testing and benchmarking those API's.

defineAnonymousClass() maybe relevant for 11u, but not mainline.

Thanks,
David

src/hotspot/share/classfile/classLoaderData.hpp Outdated Show resolved Hide resolved
@openjdk openjdk bot removed the sponsor label May 18, 2021
Copy link
Member

@stefank stefank left a comment

The identity_hash() changes look good.

FWIW, some allocators return 16 byte aligned memory chunks, so shifting by 3 might leave a non-set LSB. Maybe when fixing JDK-8267303, we should also consider if the shift should be 3 or 4.

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented May 19, 2021

/integrate

@openjdk openjdk bot added the sponsor label May 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2021

@D-D-H
Your change (at version cbed020) is now ready to be sponsored by a Committer.

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented May 19, 2021

The identity_hash() changes look good.

FWIW, some allocators return 16 byte aligned memory chunks, so shifting by 3 might leave a non-set LSB. Maybe when fixing JDK-8267303, we should also consider if the shift should be 3 or 4.

Thank you!

@iwanowww
please help sponsor it if there is no other problem, thanks in advance : )

@iwanowww
Copy link

@iwanowww iwanowww commented May 20, 2021

Tier1-tier2 results are good.

/sponsor

@openjdk openjdk bot closed this May 20, 2021
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels May 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 20, 2021

@iwanowww @D-D-H Since your change was applied there have been 224 commits pushed to the master branch:

  • 1c7a131: 8267350: Archived old interface extends interface with default method causes crash
  • 005d8a7: 8256372: [macos] Unexpected symbol was displayed on JTextField with Monospaced font
  • 81f39ed: 8261205: AssertionError: Cannot add metadata to an intersection type
  • 7b98400: 8267348: Rewrite gc/epsilon/TestClasses.java to use Metaspace with less classes
  • ca93399: 8267235: [macos_aarch64] InterpreterRuntime::throw_pending_exception messing up LR results in crash
  • b7b6acd: 8267481: Make sure table row has correct number of cells
  • f67847f: 8267396: Avoid recording "pc" in unhandled oops detector for better performance
  • 878d1b3: 8267434: Remove LinkOutput[Impl]
  • fc7f0a3: 8267480: Explicitly problemlist all runtime/os/TestTracePageSizes.java tests on linux-aarch64 to reduce noise
  • 9425d3d: 8261880: Change nested classes in java.base to static nested classes where possible
  • ... and 214 more: https://git.openjdk.java.net/jdk/compare/20ad42897427483a4a62e1de7e78d4620eb9e240...master

Your commit was automatically rebased without conflicts.

Pushed as commit 83b3607.

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

@D-D-H D-D-H deleted the JDK-8266642 branch May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-runtime integrated
5 participants