Review request for JDK-8043956: Make code caching work with optimistic typing and lazy compilation

[Hannes Wallnoefer]

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 <hannes.wallnoefer at oracle.com> 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 <hannes.wallnoefer at oracle.com <mailto:hannes.wallnoefer at oracle.com>> 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