Updated version at http://cr.openjdk.java.net/~skuksenko/jsr335/8024635/webrev.02/ according comments: - remove "null check" comment near Object.requireNonNull calls - distort/(change parameters order) in key-purposed MethodType constructor - move count-slot logic from checkPType to checkPTypes.
On 09/26/2013 11:09 PM, John Rose wrote: > (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 > -- Best regards, Sergey Kuksenko
