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.wallnoe...@oracle.com
<mailto:hannes.wallnoe...@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
