Review request for JDK-8133300: Ensure symbol table immutability in Nashorn AST

[Marcus Lagergren]

+1

> On 15 Sep 2015, at 17:29, Attila Szegedi <attila.szegedi at 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 <lagergren at gmail.com> wrote:
>> 
>> Reviewed new webrev. Happy +1
>> 
>> /M
>> 
>>> On 31 Aug 2015, at 12:09, Attila Szegedi <attila.szegedi at 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.szegedi at 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 <marcus at 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.szegedi at 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.
>>>>> 
>>>> 
>>> 
>> 
>