Re: ClassValue perf?
On 05/30/2016 06:59 PM, Peter Levart wrote: > I also employed get-acquire/put-release memory ordering semantics > instead of SC (volatile) in hope that it might improve a bit the > performance on platforms such as PowerPC or ARM, but this can be changed > back to SC if anyone gets scared of it :-) Revert, you're playing with fire here. Your _default_ modus operandi should be "scared" when dealing with concurrency. The correctness with acq/rel should be proven separately, and not by empirical testing. There is a reason why putOrdered is not used everywhere. 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: 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): 8057020: LambdaForm caches should support eviction
On 12/01/2014 07:58 PM, Vladimir Ivanov wrote: > http://cr.openjdk.java.net/~vlivanov/8057020/webrev.00/ > https://bugs.openjdk.java.net/browse/JDK-8057020 Looks okay, although the cache management logic gives me a headache after the vacation. I thought I spotted a few bugs, but those were only false positives. > The fix is to use SoftReferences to keep LambdaForms alive as long as > possible, but avoid throwing OOME until the caches are evicted. I > experimented with WeakReferences, but it doesn't hold LambdaForms for > long enough: LambdaForm cache hit rate degrades significantly and it > negatively affects application startup and warmup, since every > instantiated LambdaForm is precompiled to bytecode before usage. > > Testing: jdk/java/lang/invoke/LFCache in stress mode + jck > (api/java_lang/invoke), jdk/java/lang/invoke, jdk/java/util/streams, octane SoftReferences are tricky in the way they can get suddenly drop the referent, and normal testing would not catch it (e.g. the normal operation would reclaim softrefs under your feet almost never). Does this code survive with -XX:SoftRefLRUPolicyMSPerMB=0? 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::, 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 (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::, 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, 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] 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 (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 (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: [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 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 07:02 PM, Vladimir Ivanov wrote: > http://cr.openjdk.java.net/~vlivanov/8058309/webrev.00/ Looks good, thanks! Sorry for not being clear earlier, but I am a bit concerned with the bug synopsis: we have sure worked around the issue with LambdaForms, but are we sure this fixed the general problem? John asked me to follow up on that with more concrete benchmark, and I will do that later. In other words, I would like to see the separate issue submitted for this workaround, and record the fix under that issue. Thanks, -Aleksey. ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: [9] RFR (S) 8057654: Extract checks performed during MethodHandle construction into separate methods
On 09/05/2014 12:09 PM, Vladimir Ivanov wrote: > http://cr.openjdk.java.net/~vlivanov/8057654/webrev.00/ > https://bugs.openjdk.java.net/browse/JDK-8057654 Random style rant of the week, not particularly about this concrete patch. Can we please try to systematically use more readable/robust/secure idioms? E.g.: a) Always have curly braces around the blocks? if (ok && ...) { ok = false; } if (!ok) { throw misMatchedTypes(...); } return rtype; vs. if (ok && ...) ok = false; if (!ok) throw misMatchedTypes(...); return rtype; Apple's "goto fail;" bug, anyone? b) Have only a single initialization per line? boolean match = true; boolean fail = false; vs. boolean match = true, fail = false; c) Always have parentheses in ternary operators predicates? int foldVals = (rtype == void.class) ? 0 : 1; vs. int foldVals = rtype == void.class ? 0 : 1; 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: 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/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
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/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
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
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 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 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
(sorry for not replying to original message, just subscribed) This feels outright wrong: +static Map 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