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

[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?

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?

You’ve got JavaDoc warnings for public methods in FunctionInitializer.java 

Please explain the “sourceURLDirective” field. 

Please explain the “callSiteType” field that has been added to CompiledFunction. How was this represented before?

SpillProprerty - you are missing final for initMethodHandles param

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.

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.

What’s the deal with the dropped strict param in test/script/trusted/JDK-8006529.js?

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

Type. Isn’t ‘Z’ a better boolean descriptor as it corresponds to what it’s called in Java. ‘B’ can be confused with ‘byte’

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));

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?

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.


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> wrote:

> Please review JDK-8043956: Make code caching work with optimistic typing and lazy compilation:
> 
> http://cr.openjdk.java.net/~hannesw/8043956/webrev.01/
> 
> Thanks,
> Hannes