(Quick note on format: I have changed the subject line to include the conventional bug number and size estimate. The bug number makes it easier to track in mailers.)
On Sep 26, 2013, at 9:22 AM, Sergey Kuksenko <sergey.kukse...@oracle.com> wrote: > Hi All, > > I updated fix. > You may find it here > http://cr.openjdk.java.net/~skuksenko/jsr335/8024635/webrev.01/ > > See my comments and new webrev descition below > > On 09/18/2013 12:59 AM, John Rose wrote: >>>> We can tweak that later if necessary. There are probably only a small >>>> number of such strings, so maybe a small leaky map would do the trick as >>>> well. >>> In case of small amount of such strings we will get a huge overhead from >>> any kind of map. >> I would expect O(10^1.5) memory references and branches. Depends on what >> you mean by "huge". > Sure. I think the question to use external map or field may be decided > later when/if it will be needed. > Just some statictics, I've collected on nashorn/octane benchmarks > (statistics collected only for the single(first) benchmark iteration: > mandreel: 514 unique strings, toMethodDescriptorString called 69047 times > box2d: 560 unique strings, 34776 toMethodDescriptorString invocations. Good data; thanks. > >> Would there be any benefit from recording the original string from the >> constant pool? The JVM path for this is >> SystemDictionary::find_method_handle_type, which makes an upcall to >> MethodHandleNatives.findMethodHandleType. Currently only the ptypes and >> rtype are passed, but the JVM could also convert the internal Symbol to a >> java.lang.String and pass that also. In that case, MT's created by JVM >> upcalls would be pre-equipped with descriptor strings. >> >> This is probably worth an experiment, although it requires some JVM work. > > I am definitely sure that we shouldn't do that. > Right now caching desriptor strings is internal decision of MethodType. > Otherwise it will make interfaces more complex. Yes, I agree. Also, it would only benefit the 514 calls which introduce unique strings, whereas your change helps the 69047 calls for descriptor strings. >> I hope you get my overall point: Hand optimizations have a continuing cost, >> related to obscuring the logical structure of the code. The transforming >> engineer might make a mistake, and later maintaining engineers might also >> make mistakes. >> >> https://blogs.oracle.com/jrose/entry/three_software_proverbs > > And it doesn't mean that we should afraid any kind of optimization. > Lucky positions - someone(or something) will optimize it for us. But > sometimes JIT (even smart JIT) is not enough. Sometimes. When that happens we reluctantly hand-optimize our code. > Let's back to the patch.construstors > I decided not change original checkPType/checkRType code, except > explicit Objects.requireNonNull. The problem here that we are creating > new MethodType objects which are already exists, we have to create MT as > a key for searching in the internedTable. Ratio between already exiting > and new MethodTypes a quite huge. > Here is some statistics I've collected on nashorn/octane benchmarks > (statistics collected only for the single(first) benchmark iteration: > > mandreel: > - 1739 unique MethodTypes, > - 878414 MethodType.makeImpl requests; > box2d: > - 1861 unique MethodTypes, > - 527486 MethodType.makeImpl requests; > gameboy: > - 2062 unique MethodTypes, > - 311378 MethodType.makeImpl requests; > > So > 1. MethodType constructor do checkPTypes/checkRType which are frequently > (99%) useless - we already have the same (checked) MethodType in the > internTable. > 2. Less than 1% of created MethodTypes will be stored into internTable, > but reusing that MethodType objects prevent ti from scalar replacement. > (I've heard that Graal may do flow-sensitive scalar replacement, but C2 > can't do). > > What I did. I separate constructors: > - for long lived MethodTypes with all checks, > - for short lived MethodTypes used only as key for searching in the > InterTable, no checks are performed. > if we didn't find MethodType in the table we always create a new one > (with checks) that is required in less than 1% cases. And we remove at > least one obstacle for scalar replacement. Unfortunately, scalaer > replacement for expression "internTable.get(new MethodType(rtype, > ptypes))" was not performed, the reason may be evaluated, and hope it > would be easier to achieve scalarization in this case. Brilliant. I love this. The constructor signatures are too similar given the subtle difference between their semantics. Please distort the signature of the extra-sensitive constructor. (Imagine the wrong one accidentally invoked by helpful IDE code completers.) Put in a leading int dummy argument, or reverse the parameters, or some similar hack. (If we had named constructors, we'd hack the name instead.) I admit this is of marginal benefit, since there is only one place where the constructors are called, but it's good style. If there were more places calling the constructor, we'd absolutely want a clear distinction between checked and unchecked. If it still makes sense, I wouldn't mind moving the slot-count logic (long/double adjustment) out of checkPType into checkPTypes. But I think that's moot with this much better change. - slots += checkPtype(ptype); + checkPtype(ptype); + if (ptype == double.class || ptype == long.class) { + slots++; + } Good work. Reviewed. (You need another reviewer, I think.) — John
