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

[Hannes Wallnoefer]

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
>