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 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. 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 >> >
