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