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
