+1
On 9/15/2015 9:01 PM, Attila Szegedi wrote:
Sorry, forgot to specify the link:
http://cr.openjdk.java.net/~attila/8133300/webrev.jdk8u-dev
<http://cr.openjdk.java.net/~attila/8133300/webrev.jdk8u-dev>
On Sep 15, 2015, at 5:29 PM, Attila Szegedi <attila.szeg...@oracle.com> wrote:
Folks, I’m preparing to backport this to 8u-dev. I had to prepare a separate
webrev, as I had to change few things, namely:
- I resolved a conflict in CompilationPhase.java stemming from Sundar’s 8055917
being committed to 8u-dev before this change (this was BTW also the reason poor
Sundar himself had a conflict and had to manually prepare a 8u-dev webrev…)
- Some omitted type parameters in 9 can’t be omitted in 8 because of weaker
type parameter inference, hence some more explicit type specializations were
used in few places
- use of lambdas in RecompilableScriptFunctionData was not compatible with
Nashorn using source -1.7 in 8u-dev.
Please review so I can request a backport to 8u-dev.
Thanks,
Attila.
On Sep 1, 2015, at 9:14 AM, Marcus Lagergren <lagerg...@gmail.com> wrote:
Reviewed new webrev. Happy +1
/M
On 31 Aug 2015, at 12:09, Attila Szegedi <attila.szeg...@oracle.com> wrote:
I posted another webrev, can you please review:
http://cr.openjdk.java.net/~attila/8133300/webrev.01.jdk9
<http://cr.openjdk.java.net/~attila/8133300/webrev.01.jdk9>
The only changes are:
- added JavaDoc comments explaining SerializedAst class, as per Marcus’
suggestion
- external symbols’ re-marking as global now only happens for non-cached split
functions (previously, it happened for all non-cached functions). Other
functions will already have external symbols marked as globals, as they’re
coming from a lazy compilation.
Thanks,
Attila.
On Aug 31, 2015, at 10:58 AM, Attila Szegedi <attila.szeg...@oracle.com> wrote:
if the FunctionNode object is set as the cached representation of a
RecompilableScriptFunctionData using its setCached method, its IS_CACHED flag
is set to true. The flag is used in recompilation to determine the necessary
compilation phases: if the function is cached, it means phases in
Compiler.COMPILE_UPTO_CACHED were already run on it, so they don't need to be
repeated. Otherwise the function is freshly reparsed, so the full compiler
pipeline needs to be re-run.
There’s also the issue of re-marking external symbols as globals in
RecompilableScriptFunctionData.cloneSymbols(). We normally don’t have to do
that – I implemented that back when I was caching pre-pass ASTs. Now that we're
only caching AST resulting from lazy compilation, we don't really need it
except for split functions, which are incidentally cached from the eager pass
:-)
When we lazily compile a function from scratch, all symbols outside of it are considered
global (since AssignSymbols won't find them in the enclosing lexical scope, as the
enclosing source text is not reparsed). OTOH, if we cache a FunctionNode resulting from
the eager first pass, it will have the full lexical scope, so those symbols won't be
marked as "global" (which is rather a misnomer at the moment and should be
instead considered “external” instead). We need the cached AST and a lazily parsed AST to
be identical though, otherwise things break down further down the line.
Split functions are cached from eager pre-pass (and before they were serialized
from eager pre-pass). They didn't suffer from this problem though as
AssignSymbols was re-run every time after deserialization. Now however, I moved
AssignSymbols to be a pre-cache phase.
I'll modify the code though so that this marking of external symbols only
happens when we cache a split function.
Attila.
On Aug 30, 2015, at 3:07 PM, Marcus Lagergren <mar...@lagergren.net> wrote:
Can you elaborate a bit on the isCached mechanism in FunctionNode?
/M
On 26 Aug 2015, at 14:18, Attila Szegedi <attila.szeg...@oracle.com> wrote:
Please review JDK-8133300 "Ensure symbol table immutability in Nashorn AST" at
<http://cr.openjdk.java.net/~attila/8133300/webrev.jdk9> for
<https://bugs.openjdk.java.net/browse/JDK-8133300>
Implementation notes for reviewers are here:
<https://bugs.openjdk.java.net/browse/JDK-8133300?focusedCommentId=13837384&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13837384
<https://bugs.openjdk.java.net/browse/JDK-8133300?focusedCommentId=13837384&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13837384>>
Thanks,
Attila.
