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. >>>>> >>>> >>> >> >
