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