+1 in that case. On 05 Aug 2014, at 11:45, Hannes Wallnoefer <[email protected]> wrote:
> Am 2014-08-05 um 10:10 schrieb Marcus Lagergren: >> Very good summary Hannes - this answers all of my questions. The toStrings >> in the doPrivileged block is the only one that still worries me. >> I’ll be happy to look at a new webrev, but I don’t think I have a lot to say. >> >> Actually one more thing - so this is now a generic mechanism for storing and >> persisting all code, no matter if it’s optimistic or eager? Does it use >> Attila’s optimistic type storage, or is it new through and through? I see >> you have various weird dependencies between the code caches now > > Yes, code store now works for both optimistic and non-optimistic. > > While working on this I had it unified with Attila's optimistic type storage, > using the exact same directory structure and naming conventions. But it > didn't feel right for various reasons. Type caching only uses very small > amounts of storage while class caching quickly runs into hundreds of MB. > Because of this, it felt wrong to use a storage location that is hidden from > the user. Also, using type caching storage wouldn't allow to override code > store behaviour which we eventually plan to allow. > > Can you be more explicit about what you mean by weird dependencies between > code caches? > > >> >> Have you checked that runtime performance is the same for e.g. Octane, given >> that you have been poking around a bit in the delicate >> AccessProperty/SpillProperty getters and setters. > > I haven't noticed any losses of performance during my tests, will double > check to make sure. > > Hannes > >> >> Regards >> Marcus >> >> On 04 Aug 2014, at 19:00, Hannes Wallnoefer <[email protected]> >> wrote: >> >>> Hi Marcus, answers inline. >>> >>> Am 2014-08-04 um 18:20 schrieb Marcus Lagergren: >>>> Hi Hannes! >>>> >>>> I get the general gist of this and it looks nice. Some questions though, >>>> which I probably ask because I am sunstruck by the 33 C in my house. I >>>> don’t understand all parts of the change, but I do understand it >>>> architecturally fine. >>>> No particular priority or size order of questions below: >>>> >>>> —8<-- >>>> >>>> Why is the project.properties diff empty? >>> Removed trailing whitespace >>> >>>> What is the rationale of removing CompiledFunctions? I’m going to have >>>> some merge issues with this in my optimistic native code branch, but no >>>> biggies. Just curious as to how the model works instead. Is it just >>>> because it would add more serialization problems that it went away and >>>> turned into a list in ScriptFunctionData? >>> The purpuse is just to simplify the code, which moved to either >>> ScriptFunctionData or CompiledFunction. The idea is just to have >>> >>>> You’ve got JavaDoc warnings for public methods in FunctionInitializer.java >>> Will fix this. >>> >>>> Please explain the “sourceURLDirective” field. >>>> >>> We allow setting the source URL from a //@ or //# comment within the >>> source. Previously we passed this through the whole compilation as a >>> separate parameter, which caused lots of noise. >>> >>> Attila suggested to rename this to "explicitURL" and I did so in my >>> reworked patch. >>> >>>> Please explain the “callSiteType” field that has been added to >>>> CompiledFunction. How was this represented before? >>> My intention was to add a shortcut for when the callsite type is exactly >>> the same as when the function was compiled. >>> >>>> SpillProprerty - you are missing final for initMethodHandles param >>> Will fix. >>> >>>> CodeStore : missing various finals for code convention >>>> >>>> Security anti pattern - you are using implicit toStrings in a doPrivileged >>>> block in CodeStore. Not sure if the JDK classes being toStringed are safe >>>> or final, but please check it it’s a problem, or generate the string >>>> before the doPrivileged. >>> I'll check this for the next webrev. >>> >>> >>>> CodeInstaller.java - finals missing >>>> >>>> Why are the getters and setters in AccessorProperty now exposed? Because >>>> SpillProperty needs them for serialisation? Why? Why package private and >>>> not protected - I guess it’s stronger security so it should be fine. >>> Exactly. I didn't think it needed to be accissible from outside the package. >>> >>>> What’s the deal with the dropped strict param in >>>> test/script/trusted/JDK-8006529.js? >>> The param that was dropped was the sourceURL one. See my response to >>> "sourceURLDirective" question above. >>> >>>> Parser / FunctionNode - why has the setSourceURL logic gone - this is >>>> probably a question I ask because I can’t see 100% of the big picture due >>>> to heatstroke/slowness/stupidity/being almost 40. I know there’s a >>>> sourceURLDirective instead, but I am not 100 % on how it works and where >>> We now set this in source instead of passing it along as separate param. >>> >>>> Type. Isn’t ‘Z’ a better boolean descriptor as it corresponds to what it’s >>>> called in Java. ‘B’ can be confused with ‘byte’ >>> Agreed. >>> >>>> OptimisticTypesPersistence: protocol.equals(“jar”) -> >>>> “jar”.equals(protocol) to avoid potential unnecessary NullPointers. >>>> >>>> What’s the purpose of the PropertyMapWrapper? Is this just to get equals >>>> and hashCode for property maps? Not sure why it’s designed this way. I do >>>> know that it’s been around for a while and isn’t a part of this change, >>>> but I wanted to ask the question anyway to complete my mental model. >>>> >>>> Comment to be removed? + // System.err.println("ADDED PROP MAP " + >>>> System.identityHashCode(object) + " // " + Debug.caller(2, 5)); >>> Already removed. >>> >>>> Compiler: + Map<Integer, FunctionInitializer> functionInitializers; - >>>> maybe move the field to the top of the file with the other fields. Almost >>>> didn’t see it >>>> >>>> CompilationPhase.java + CompileUnit newUnit = >>>> compiler.createCompileUnit(sb.toString(), oldUnit.getWeight()); wants a >>>> final >>>> >>>> CodeGenerator - what did we use “initializedFunctionIds” for? Why is it no >>>> longer needed. Why did we need it before? Why could we even try to >>>> initialize a function twice? >>> I had to unify function initialization between eager and on-demand code and >>> newly compiled and deserialized code, which is why I moved this >>> functionality out of CodeGenerator and CompileUnit. Actually I don't think >>> initializedFunctionIds was needed before. I'll check this again and maybe >>> add an assertion or something. >>> >>>> The FunctionInitializer class frequently confuses me, due to my weak >>>> brain. I’d like to see a longer descriptive comment, perhaps with a simple >>>> use case at the top of FunctionInitializer. >>> Will add this. >>> >>> Thanks! >>> >>> Hannes >>> >>>> Architecturally, this looks very good! >>>> Thanks for bashing your forehead against this one for some time, Hannes! >>>> >>>> Regards >>>> Marcus >>>> >>>> >>>> On 04 Aug 2014, at 15:52, Hannes Wallnoefer <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> >>>>> Please review JDK-8043956: Make code caching work with optimistic typing >>>>> and lazy compilation: >>>>> >>>>> http://cr.openjdk.java.net/~hannesw/8043956/webrev.01/ >>>>> <http://cr.openjdk.java.net/%7Ehannesw/8043956/webrev.01/> >>>>> >>>>> Thanks, >>>>> Hannes >
