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