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 <[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/
>
> Thanks,
> Hannes
