Re: ClassValue perf?
On 05/19/2016 03:32 PM, Michael Haupt wrote: > It may well be that running the bechmark so few times does not deliver a > stable enough result. I'd like Aleksey to comment on this: is adopting > Peter's code worthwhile given it improves on footprint and reduces code > size and complexity? Eh, if you pose the question like that, the answer is obviously "yes". I like how Peter's version strips down the ClassValue impl. But, looking at the data, it would seem we are regressing randomAccess with low classValueCount? Benchmark (cCount) (cvCount) Mode CntScoreError Units # result-plain.txt randomAccess 1024 1 avgt 1018.375 ± 0.046 ns/op randomAccess 1024 4 avgt 1026.755 ± 0.018 ns/op randomAccess 1024 16 avgt 1026.263 ± 0.024 ns/op randomAccess 1024256 avgt 1053.543 ± 0.419 ns/op # result-plevart-03.txt randomAccess 1024 1 avgt 1023.315 ± 0.053 ns/op randomAccess 1024 4 avgt 1028.323 ± 0.053 ns/op randomAccess 1024 16 avgt 1029.514 ± 0.070 ns/op randomAccess 1024256 avgt 1045.339 ± 0.035 ns/op This seems to go the other direction Michael was pursuing: optimizing the single-value case. Seems even more pronunciated on low classCount. I'd be more happy if we can at least not regress the performance. If there is a cleaner implementation with the same perf characteristics, I'd be inclined to accept it, of course. > I agree regarding whether there's a point in optimising for single-value > storage whilst maintaining full flexibility. In a scenario where it is > known that only one value will be associated with a class, it's better > to use static fields. Specialized solutions that can use the knowledge about the external condition would always win, given enough effort. The improvements in shared infrastructure are still very welcome, because they break the chicken-and-egg problem: you would not use a shared API if it is slow, and you would not optimize shared API because nobody uses it. Thanks, -Aleksey signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9] RFR (M): 8057967: CallSite dependency tracking scales devastatingly poorly
On 04/01/2015 11:56 PM, Vladimir Ivanov wrote: http://cr.openjdk.java.net/~vlivanov/8057967/webrev.00/hotspot/ http://cr.openjdk.java.net/~vlivanov/8057967/webrev.00/jdk/ https://bugs.openjdk.java.net/browse/JDK-8057967 Glad to see this finally addressed, thanks! I did not look through the code changes, but ran Octane on my configuration. As expected, Typescript had improved substantially. Other benchmarks are not affected much. This in line with the performance analysis done for the original bug report. Baseline: Benchmark Mode CntScoreError Units Box2D.test ss 20 4454.677 ±345.807 ms/op CodeLoad.testss 20 4784.299 ±370.658 ms/op Crypto.test ss 20 878.395 ± 87.918 ms/op DeltaBlue.test ss 20 502.182 ± 52.362 ms/op EarleyBoyer.test ss 20 2250.508 ±273.924 ms/op Gbemu.test ss 20 5893.102 ±656.036 ms/op Mandreel.testss 20 9323.484 ±825.801 ms/op NavierStokes.testss 20 657.608 ± 41.212 ms/op PdfJS.test ss 20 3829.534 ±353.702 ms/op Raytrace.testss 20 1202.826 ±166.795 ms/op Regexp.test ss 20 156.782 ± 20.992 ms/op Richards.testss 20 324.256 ± 35.874 ms/op Splay.test ss 20 179.660 ± 34.120 ms/op Typescript.test ss 20 40.537 ± 2.457 s/op Patched: Benchmark Mode CntScoreError Units Box2D.test ss 20 4306.198 ±376.030 ms/op CodeLoad.testss 20 4881.635 ±395.585 ms/op Crypto.test ss 20 823.551 ±106.679 ms/op DeltaBlue.test ss 20 490.557 ± 41.705 ms/op EarleyBoyer.test ss 20 2299.763 ±270.961 ms/op Gbemu.test ss 20 5612.868 ±414.052 ms/op Mandreel.testss 20 8616.735 ±825.813 ms/op NavierStokes.testss 20 640.722 ± 28.035 ms/op PdfJS.test ss 20 4139.396 ±373.580 ms/op Raytrace.testss 20 1227.632 ±151.088 ms/op Regexp.test ss 20 169.246 ± 34.055 ms/op Richards.testss 20 331.824 ± 32.706 ms/op Splay.test ss 20 168.479 ± 23.512 ms/op Typescript.test ss 20 31.181 ± 1.790 s/op The offending profile branch (Universe::flush_dependents_on) is also gone, which explains the performance improvement. Thanks, -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9, 8u40] RFR (M): 8063135: Enable full LF sharing by default
Looks good now. -Aleksey. On 11/19/2014 01:30 PM, Vladimir Ivanov wrote: Hm, I remember I fixed that long time ago... Seems like I chose a stale patch. Sorry for that. Updated webrev in place. Best regards, Vladimir Ivanov On 11/19/14, 3:38 AM, Aleksey Shipilev wrote: On 11/18/2014 11:23 PM, Vladimir Ivanov wrote: http://cr.openjdk.java.net/~vlivanov/8063135/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8063135 Broken array index here: 69 TRACE_METHOD_LINKAGE = (Boolean) values[3]; 70 COMPILE_THRESHOLD = (Integer) values[5]; Also, how does it pass the tests? It should fail with AIOBE during MethodHandleStatics::clinit, since 53 final Object[] values = new Object[7]; ... 72 PROFILE_LEVEL = (Integer) values[7]; Thanks, -Aleksey. ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9, 8u40] RFR (XXS): 8059880: Get rid of LambdaForm interpretation
On 11/19/2014 12:01 AM, Vladimir Ivanov wrote: http://cr.openjdk.java.net/~vlivanov/8059880/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8059880 Yes, for the love of God, GO FOR IT. -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9, 8u40] RFR (M): 8063135: Enable full LF sharing by default
On 11/18/2014 11:23 PM, Vladimir Ivanov wrote: http://cr.openjdk.java.net/~vlivanov/8063135/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8063135 Broken array index here: 69 TRACE_METHOD_LINKAGE = (Boolean) values[3]; 70 COMPILE_THRESHOLD = (Integer) values[5]; Also, how does it pass the tests? It should fail with AIOBE during MethodHandleStatics::clinit, since 53 final Object[] values = new Object[7]; ... 72 PROFILE_LEVEL = (Integer) values[7]; Thanks, -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9] RFR (S): 8058892: FILL_ARRAYS and ARRAYS are eagely initialized in MethodHandleImpl
I have three comments:
* Since initialization order is important, why don't put the
initialization in the existing static initializer? This will secure for
inadvertent field reordering in future.
* Any reason two new fields are private? All other seem package-private.
* Any performance problems if we actually count FILL_ARRAYS_COUNT in
during the static initialization, instead of putting a magic value?
-Aleksey.
On 10/02/2014 08:55 PM, Vladimir Ivanov wrote:
Small update:
http://cr.openjdk.java.net/~vlivanov/8058892/webrev.01/
Need to reorder initialization sequence in MHI.Lazy. Initialized
FILL_ARRAYS and ARRAYS are required for later MH lookups.
Additional testing:
* jck (api/java_lang/invoke)
* jdk/java/lang/invoke, jdk/java/util/streams w/ -ea -esa and
COMPILE_THRESHOLD={0,30}
Best regards,
Vladimir Ivanov
On 10/2/14, 7:52 PM, Vladimir Ivanov wrote:
http://cr.openjdk.java.net/~vlivanov/8058892/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8058892
Core j.l.i classes are preloaded during VM startup in order to avoid
possible deadlock when accessing JSR292-related functionality from
multiple threads. After LF sharing-related changes, FILL_ARRAYS and
ARRAYS are initialized too early. It affects startup time footprint of
applications that don't use JSR292.
The fix is to move these fields into MHI.Lazy class, thus delaying their
initialization to the first usage of JSR292 API.
Testing: failing test, manual (measured HelloWorld app startup time;
compared -XX:+PrintCompilation logs)
Best regards,
Vladimir Ivanov
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
signature.asc
Description: OpenPGP digital signature
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9] RFR (S): 8058892: FILL_ARRAYS and ARRAYS are eagely initialized in MethodHandleImpl
On 10/02/2014 09:26 PM, Vladimir Ivanov wrote: Aleksey, Thanks for the review. Updated version: http://cr.openjdk.java.net/~vlivanov/8058892/webrev.02/ Looks good. * Since initialization order is important, why don't put the initialization in the existing static initializer? This will secure for inadvertent field reordering in future. Good idea. Fixed. * Any reason two new fields are private? All other seem package-private. These fields are intended for usage only in MHI.varargsArray/buildFiller. They were private before and I decided to keep that. It is fine for MH_*/NF_* to be used from other places in j.l.i (e.g. NF_checkSpreadArgument is used in LambdaFormEditor). Okay. My sentiment was on par with Remi's: needless access methods for private fields. Now that these fields are in Lazy, the access methods are generated when the fields are accessed from the enclosing class (MethodHandleImpl). * Any performance problems if we actually count FILL_ARRAYS_COUNT in during the static initialization, instead of putting a magic value? It's more about code structure than performance. There's a dependency between FILL_ARRAYS size LEFT_ARGS. If FILL_ARRAYS_COUNT is moved to Lazy, LEFT_ARGS should be moved to Lazy as well or LEFT_ARGS should be lazily initialized separately. IMO it doesn't worth an effort. I am not talking about moving FILL_ARRAYS_COUNT to Lazy. I am talking about computing the FILL_ARRAYS_COUNT in the new static initializer of MethodHandleImpl itself -- so that we would not get bugs when adding new fillArray override. But, given there are asserts in the code, it seems excessive. Leave it as is. -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9] RFR (XXS): 8058309: Unsafe.defineAnonymousClass deoptimization checks scale devastatingly poorly
On 09/17/2014 08:43 PM, Vladimir Ivanov wrote: I don't see anything obviously wrong either with U.dAC() or with dependency tracking in VM. What we stumbled upon is an inherent limitation of current dependency tracking implementation. Yes, and so the question from John, which I need to follow up on, if we need to fix that implementation generically, instead of avoiding the problem with point fixes :) It's not specific to U.dAC(). Regular class loaders can hit similar problem as well. Even better, this is even more generic. If you want to use 8058309 for dependency tracking improvments in VM, let me know. So far, I got an impression it is about LFs JSR292 mostly. Yes, please commit the LF fix under the different bug ID, if that is not a hassle? -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9] RFR (XXS): 8058309: Unsafe.defineAnonymousClass deoptimization checks scale devastatingly poorly
On 09/17/2014 08:55 PM, Vladimir Ivanov wrote: It's not specific to U.dAC(). Regular class loaders can hit similar problem as well. Even better, this is even more generic. Please, update bug synopsis then. Thanks, did so. Also reassigned the bug, and lowered the priority. Done: 8058661: Compiled LambdaForms should inherit from Object to improve class loading performance https://bugs.openjdk.java.net/browse/JDK-8058661 Awesome. I have no further comments, the patch is plain and simple. -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9] RFR (XXS): 8058309: Unsafe.defineAnonymousClass deoptimization checks scale devastatingly poorly
On 09/17/2014 10:53 PM, Remi Forax wrote: On 09/17/2014 06:55 PM, Vladimir Ivanov wrote: It's not specific to U.dAC(). Regular class loaders can hit similar problem as well. Even better, this is even more generic. Please, update bug synopsis then. If you want to use 8058309 for dependency tracking improvments in VM, let me know. So far, I got an impression it is about LFs JSR292 mostly. Yes, please commit the LF fix under the different bug ID, if that is not a hassle? Done: 8058661: Compiled LambdaForms should inherit from Object to improve class loading performance https://bugs.openjdk.java.net/browse/JDK-8058661 I wonder if the Proxy generation code (j.l.r.Proxy) and the method accessor generation code (j.l.r.Method) doesn't have the very same issue. We'll see, Remi, in the course of the original issue: https://bugs.openjdk.java.net/browse/JDK-8058309 Thanks for pointing two new opportunities too look for! -Aleksey signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: Instrumenting call sites.
On 11/29/2012 11:15 PM, MacGregor, Duncan (GE Energy Management) wrote: Thanks for any help you can give., Probably unrelated, but can't you do the same thing by dumping the type profile from HotSpot? -XX:+TraceTypeProfile (I think it requires debug build at this point, but 7u12 onwards should have that available in production builds). -Aleksey. ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: MethodHandle.bindTo() only for reference types?
Hi John, On 07/24/2012 11:02 PM, John Rose wrote: On Jul 24, 2012, at 11:09 AM, Attila Szegedi wrote: MethodHandle.bind is a less-general primitive. The general API is insertArguments. Good 292 support for primitives requires a signature-polymorphic API. Therefore, we have been considering adding something like this, to fill functionality not covered by bind and insertArguments: Thanks for thinking forward on this. But let's get back to the original issue: inconsistent API between MH.bindTo() and MHs.insertArguments(). If the latter boxes primitive values, why not to allow the former to do the same? Albeit it is not a good support for primitives, but this is the consistency improvement. Thanks, Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/23/2012 10:31 PM, John Rose wrote: On Jul 23, 2012, at 2:27 AM, Aleksey Shipilev wrote: The code does not need to be scalable, because the number of entries in the cache is small (order of 10-100) and scales only with type schema complexity, not workload complexity. If I had a nickel... Not sure if this logic is applicable in this particular case. This is the potential performance cliff you are eager to get rid of with new implementation. Given enough users and applications make use of this code, someone will eventually step and burn on this. This wishful thinking it's okay to use synchronized here, because this couldn't possibly get contended lead to many beautiful scalability bottlenecks throughout the JDK. While it is usually a simple thing to fix, I'm keen on not allowing to make the same mistakes over and over again. So in this case, static synchronized is the correct choice. I shall wait for mainline integration to complete and then try to run the microbenchmarks against the new backend; will see if this potential issue is the practical one. -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
MethodHandle.bindTo() only for reference types?
Hi, I wonder if anyone could point to the explanation why MH.bindTo() is accepting only reference types? This behavior seems surprising for newcomers like me, mostly because similar API accepts primitive types. For one, given: public static void foo(int i); ...and the MethodHandle mh referring to it, I can do: MethodHandle mh2 = MethodHandles.insertArguments(mh, 0, (int)someInt) ...but this one is prohibited: MethodHandle mh2 = mh.bindTo((int)someInt); Thanks, -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/24/2012 07:15 PM, Christian Thalinger wrote: Not sure if this logic is applicable in this particular case. This is the potential performance cliff you are eager to get rid of with new implementation. No it's not. We know exactly what causes the performance cliff. It's a completely unrelated issue in the VM. Sorry for misguided definition there, thought quoting does the trick for me. I was meant to say that the scalability bottlenecks like these pop out quickly, unexpected and hit hard. The difference between single-threaded and two-threaded versions could be so dramatic, so you can easily say it goes down the sink. It's inconvenient to fix one cliff and introduce the other, albeit completely unrelated. -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/22/2012 03:45 AM, John Rose wrote:
On Jul 18, 2012, at 1:43 AM, Aleksey Shipilev wrote:
Yes.
Thanks John. I'm having a glance over the fix in new webrev, and this
feels even worse:
+ private static SpeciesData get(String types) {
+// Acquire cache lock for query.
+SpeciesData d = lookupCache(types);
+if (!d.isPlaceholder())
+return d;
+Class? extends BoundMethodHandle cbmh;
+synchronized (d) {
+// Use synch. on the placeholder to prevent multiple
instantiation of one species.
+// Creating this class forces a recursive call to
getForClass.
+cbmh = Factory.generateConcreteBMHClass(types);
+}
+// Reacquire cache lock.
+d = lookupCache(types);
+// Class loading must have upgraded the cache.
+assert(d != null !d.isPlaceholder());
+return d;
+}
+ static SpeciesData getForClass(String types, Class? extends
BoundMethodHandle clazz) {
+// clazz is a new class which is initializing its
SPECIES_DATA field
+return updateCache(types, new SpeciesData(types, clazz));
+}
+ private static synchronized SpeciesData lookupCache(String types) {
+SpeciesData d = CACHE.get(types);
+if (d != null) return d;
+d = new SpeciesData(types);
+assert(d.isPlaceholder());
+CACHE.put(types, d);
+return d;
+}
+ private static synchronized SpeciesData updateCache(String types,
SpeciesData d) {
+SpeciesData d2;
+assert((d2 = CACHE.get(types)) == null || d2.isPlaceholder());
+assert(!d.isPlaceholder());
+CACHE.put(types, d);
+return d;
+}
Global synchronization is the performance smell, and this looks to be
potential scalability bottleneck (it sends shivers down my spine every
time I see static synchronized in the same line. That is not to
mention synchronizing on placeholder looks suspicious and error-prone.
Do you need help rewriting this with CHM?
-Aleksey.
signature.asc
Description: OpenPGP digital signature
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/22/2012 04:16 AM, John Rose wrote: P.S. If there's something you don't like in one of the files, let us know. As I noted before, we can (and will) roll more adjustments into the next push. I have a question about $PREPARED_FORMS there. It looks like it is not used anywhere in the code, should we eliminate it whatsoever? If not, then I have trouble understanding if we need to explicitly override CHM defaults, especially concurrency level there. (I know Doug pushes back on specialized constructors in CHMv8 because some of the parameters are meaningless there). -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
(sorry for not replying to original message, just subscribed)
This feels outright wrong:
+static MapString, Data CACHE = new IdentityHashMap();
+
+static Data get(String types) {
+final String key = types.intern();
+Data d = CACHE.get(key);
+if (d == null) {
+d = make(types);
+Data e = CACHE.get(key);
+if (e != null) {
+d = e;
+} else {
+CACHE.put(key, d);
+}
+}
+return d;
+}
I couple of questions pops out in my mind:
1. Is this code supposed to be thread-safe? It is then wrong to use
unguarded IdentityHashMap without external synchronization.
2. Do we really need a second .get() check, if code is not thread-safe
anyway? This code allows two threads to call put() on the same key anyway.
3. Do the $types *need* to be interned? Or is this the premature
optimization (gone wrong, btw)?
I would rather like to see CHM.putIfAbsent()-based cache on plain
non-interned strings there. Even if interned strings are required, we
could intern them on successful map update, not every time we look up
the key.
-Aleksey.
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
