RFR: 8221393: ResolvedMethodTable too small for StackWalking applications

[coleen.phillimore at oracle.com]

http://cr.openjdk.java.net/~stefank/8221393/webrev.01/src/hotspot/share/classfile/javaClasses.cpp.udiff.html

+ if (method->is_old()) {
+ // Replace method with redefined version
+
+ method = holder->method_with_idnum(method->method_idnum());
+ if (method == NULL) {
+ // Replace deleted method with NSME.
+ method = Universe::throw_no_such_method_error();
+ }
+ }


This code that came from resolvedMethodTable.cpp was just changed with 
https://bugs.openjdk.java.net/browse/JDK-8221992.  Can you use the new 
code here.

But make sure this holder is the one of the new method selected:

+ holder->set_has_resolved_methods();


http://cr.openjdk.java.net/~stefank/8221393/webrev.01/test/hotspot/jtreg/runtime/testlibrary/ClassWithManyMethodsClassLoader.java.html

    2  * Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights reserved.


Copyright is wrong.   Looks like a good utility class we can use.

The rest looks great!


On 4/5/19 8:24 AM, Stefan Karlsson wrote:
> Hi all,
>
> Please review this patch to replace the hashtable in 
> ResolvedMethodTable, with the ConcurrentHashTable (currently used by 
> the StringTable and SymbolTable).
>
> http://cr.openjdk.java.net/~stefank/8221393/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8221393
>
> This solves at least two scalability problems with the 
> ResolvedMethodTable:
>
> 1) The old table had a fixed, small size that could cause the table to 
> be overpopulated when StackWalking APIs were used. See the bug report. 
> The new table automatically grows when it gets filled up.
>
> 2) The old table used locks for inserts, lookups, and cleanups. This 
> could cause large performance degradations when multiple threads used 
> the StackWalking APIs, or resolved MethodHandle constants. The new 
> table uses lock-free inserts and lookups, and thereby scales much better.
>
> A stack walking micro benchmark (not yet pushed) shows this.
>
> Running with 8 threads, on my 16 core machine:
> java -XX:+UseG1GC -Xmx1g -Xms1g -jar benchmarks.jar -wi 0 -i 5 -p 
> depth=4 -t 8 StackWalkBench.swClassNamesDefaultOpts
>
> Baseline: avgt 25 91777.142 ± 2760.507  ns/op
> Change:   avgt 25  8149.368 ± 212.863   ns/op
>
> Note that a lot of the code to use the ConcurrentHashTable is copied 
> from the StringTable and/or SymbolTable. There are unifications, and 
> maybe simplifications that we can do as follow-up RFEs.
>

I agree.  I'll file some RFEs unless you do it.

> There's an open question regarding the verification code in 
> ResolvedMethodTable::finish_dead_counter(). It's a quadratic 
> verification operation, and has the potential to take a long time. I 
> left it in for testing, but I don't think we should have it always 
> enabled. For the StringTable we have VerifyStringTableAtExit, to do 
> verification before the JVM shuts down. Any suggestions on what to do 
> for the ResolvedMethodTable?
>

For the most part, RMT is pretty small unless stackwalk API is used.  I 
would take the verification out of GC, since we don't verify string 
table that way.  Since VerifyStringTableAtExit is diagnostic, we could 
remove it and make it VerifyConcurrentHashtablesAtExit.  I'm not sure 
how useful it is here.  Maybe we could VerifyConcurrentHashtables at 
full GCs instead?

Thank you for making this change!
Coleen

> Tested with tier1-3, tier1-7 on Linux.
>
> Thanks,
> StefanK