Re: Review request for JDK-8193371: Use Dynalink REMOVE operation in Nashorn

[Hannes Wallnöfer]

Nice work indeed.

+1

Hannes

> Am 14.12.2017 um 12:42 schrieb Attila Szegedi :
> 
> Please review JDK-8193371 "Use Dynalink REMOVE operation in Nashorn" at 
>  for 
> 
> 
> It looks bigger than it is. let me quickly explain this change in detail:
> 
> There are some things that could make sense even if we didn’t refit delete 
> operator on Dynalink REMOVE.
> 
> * Fist such thing is moving the evaluation logic for the delete operator from 
> AssignSymbols into CodeGenerator. That way, we eliminated a need to carry 
> information from AS to CG in a RuntimeNode, and we could retire all of 
> DELETE, SLOW_DELETE, and FAIL_DELETE RuntimeNode enums. It also allowed us to 
> have cleaner expression of the code generator, e.g strict flag is handled 
> more elegantly when deleting an IdentNode, and also we can just emit 
> invocation of ScriptObject.delete on the scope object for IdentNode removals. 
> There’s overall stronger typing (e.g. slow delete implementation has a better 
> signature).
> 
> * By not erasing “delete” UnaryNodes in AssignSymbols anymore, I had to take 
> a bit of special care in LocalVariableTypesCalculator to not treat deletes of 
> IdentNodes as uses.
> 
> * The changes in control flow of AbstractJavaLinker and BeanLinker 
> getGuardedInvocationComponent are actual bug fixes that became apparent to me 
> when I realized while writing the tests that the code as it was was failing 
> to link REMOVE:PROPERTY|ELEMENT; it was incorrectly not trying all the 
> namespaces it should’ve. (The bug was more generic than just that one 
> operation combination: it’d affect e.g. SET:METHOD|PROPERTY too and some 
> other combinations).
> 
> As for actual delete operator through Dynalink:
> 
> * MethodEmitter gained methods for emitting dynamic call sites for removal. 
> Those are very similar to existing ones for dynamic getters and setters, 
> nothing special there.
> 
> * The logic formerly in ScriptRuntime.DELETE has now been spread into 
> relevant linkers: ScriptObject.lookup, JSObjectLinker, Undefined.lookup, and 
> NashornBottomLinker. The case in ScriptRuntime.DELETE for “if 
> (JSType.isPrimitive(obj))” doesn’t even have to be handled separately, 
> primitive linking that instantiates wrappers takes care nicely of it.
> 
> * That said, ScriptRuntime.DELETE had a final “return true” fallback for 
> POJOs, and I decided to improve on it. For one thing, theres an automatic 
> improvement because delete now works as expected on Java lists and maps just 
> by virtue of Dynalink providing that functionality. Another improvement that 
> I decided to introduce manually is found in the way NashornBottomLinker 
> handles delete on POJOs, specifically the uses of isNonConfigurableProperty. 
> I tried treating all properties and methods of POJOs as if they were 
> non-configurable properties (we try to treat them like that elsewhere in the 
> code as well), so attempts to delete them will return false (non-strict) or 
> throw a TypeError (strict), in accordance with ES 8.12.7 [[Delete]]. The 
> newly added test file tries to exercise all new behavior.
> 
> * Finally, NashornCallSiteDescriptor now needs 4 bits for the operation as we 
> went from 8 operations to 10 with “delete obj.name” and “delete obj[index]". 
> This again halved the number of program points available to 131072, but 
> that's fortunately still about 10x what is really needed, so we should be 
> good.
> 
> Thanks,
>  Attila.
> 

Re: Review request for JDK-8193371: Use Dynalink REMOVE operation in Nashorn

[Sundararajan Athijegannathan]

+1

Nice work!

-Sundar

On 14/12/17, 5:12 PM, Attila Szegedi wrote:

Please review JDK-8193371 "Use Dynalink REMOVE operation in Nashorn" 
at  
for

It looks bigger than it is. let me quickly explain this change in detail:

There are some things that could make sense even if we didn’t refit delete 
operator on Dynalink REMOVE.

* Fist such thing is moving the evaluation logic for the delete operator from 
AssignSymbols into CodeGenerator. That way, we eliminated a need to carry 
information from AS to CG in a RuntimeNode, and we could retire all of DELETE, 
SLOW_DELETE, and FAIL_DELETE RuntimeNode enums. It also allowed us to have 
cleaner expression of the code generator, e.g strict flag is handled more 
elegantly when deleting an IdentNode, and also we can just emit invocation of 
ScriptObject.delete on the scope object for IdentNode removals. There’s overall 
stronger typing (e.g. slow delete implementation has a better signature).

* By not erasing “delete” UnaryNodes in AssignSymbols anymore, I had to take a 
bit of special care in LocalVariableTypesCalculator to not treat deletes of 
IdentNodes as uses.

* The changes in control flow of AbstractJavaLinker and BeanLinker 
getGuardedInvocationComponent are actual bug fixes that became apparent to me 
when I realized while writing the tests that the code as it was was failing to 
link REMOVE:PROPERTY|ELEMENT; it was incorrectly not trying all the namespaces 
it should’ve. (The bug was more generic than just that one operation 
combination: it’d affect e.g. SET:METHOD|PROPERTY too and some other 
combinations).

As for actual delete operator through Dynalink:

* MethodEmitter gained methods for emitting dynamic call sites for removal. 
Those are very similar to existing ones for dynamic getters and setters, 
nothing special there.

* The logic formerly in ScriptRuntime.DELETE has now been spread into relevant 
linkers: ScriptObject.lookup, JSObjectLinker, Undefined.lookup, and 
NashornBottomLinker. The case in ScriptRuntime.DELETE for “if 
(JSType.isPrimitive(obj))” doesn’t even have to be handled separately, 
primitive linking that instantiates wrappers takes care nicely of it.

* That said, ScriptRuntime.DELETE had a final “return true” fallback for POJOs, 
and I decided to improve on it. For one thing, theres an automatic improvement 
because delete now works as expected on Java lists and maps just by virtue of 
Dynalink providing that functionality. Another improvement that I decided to 
introduce manually is found in the way NashornBottomLinker handles delete on 
POJOs, specifically the uses of isNonConfigurableProperty. I tried treating all 
properties and methods of POJOs as if they were non-configurable properties (we 
try to treat them like that elsewhere in the code as well), so attempts to 
delete them will return false (non-strict) or throw a TypeError (strict), in 
accordance with ES 8.12.7 [[Delete]]. The newly added test file tries to 
exercise all new behavior.

* Finally, NashornCallSiteDescriptor now needs 4 bits for the operation as we went 
from 8 operations to 10 with “delete obj.name” and “delete obj[index]". This 
again halved the number of program points available to 131072, but that's 
fortunately still about 10x what is really needed, so we should be good.

Thanks,
   Attila.

Review request for JDK-8193371: Use Dynalink REMOVE operation in Nashorn

[Attila Szegedi]

Please review JDK-8193371 "Use Dynalink REMOVE operation in Nashorn" at 
 for 


It looks bigger than it is. let me quickly explain this change in detail:

There are some things that could make sense even if we didn’t refit delete 
operator on Dynalink REMOVE.

* Fist such thing is moving the evaluation logic for the delete operator from 
AssignSymbols into CodeGenerator. That way, we eliminated a need to carry 
information from AS to CG in a RuntimeNode, and we could retire all of DELETE, 
SLOW_DELETE, and FAIL_DELETE RuntimeNode enums. It also allowed us to have 
cleaner expression of the code generator, e.g strict flag is handled more 
elegantly when deleting an IdentNode, and also we can just emit invocation of 
ScriptObject.delete on the scope object for IdentNode removals. There’s overall 
stronger typing (e.g. slow delete implementation has a better signature).

* By not erasing “delete” UnaryNodes in AssignSymbols anymore, I had to take a 
bit of special care in LocalVariableTypesCalculator to not treat deletes of 
IdentNodes as uses.

* The changes in control flow of AbstractJavaLinker and BeanLinker 
getGuardedInvocationComponent are actual bug fixes that became apparent to me 
when I realized while writing the tests that the code as it was was failing to 
link REMOVE:PROPERTY|ELEMENT; it was incorrectly not trying all the namespaces 
it should’ve. (The bug was more generic than just that one operation 
combination: it’d affect e.g. SET:METHOD|PROPERTY too and some other 
combinations).

As for actual delete operator through Dynalink:

* MethodEmitter gained methods for emitting dynamic call sites for removal. 
Those are very similar to existing ones for dynamic getters and setters, 
nothing special there.

* The logic formerly in ScriptRuntime.DELETE has now been spread into relevant 
linkers: ScriptObject.lookup, JSObjectLinker, Undefined.lookup, and 
NashornBottomLinker. The case in ScriptRuntime.DELETE for “if 
(JSType.isPrimitive(obj))” doesn’t even have to be handled separately, 
primitive linking that instantiates wrappers takes care nicely of it.

* That said, ScriptRuntime.DELETE had a final “return true” fallback for POJOs, 
and I decided to improve on it. For one thing, theres an automatic improvement 
because delete now works as expected on Java lists and maps just by virtue of 
Dynalink providing that functionality. Another improvement that I decided to 
introduce manually is found in the way NashornBottomLinker handles delete on 
POJOs, specifically the uses of isNonConfigurableProperty. I tried treating all 
properties and methods of POJOs as if they were non-configurable properties (we 
try to treat them like that elsewhere in the code as well), so attempts to 
delete them will return false (non-strict) or throw a TypeError (strict), in 
accordance with ES 8.12.7 [[Delete]]. The newly added test file tries to 
exercise all new behavior.

* Finally, NashornCallSiteDescriptor now needs 4 bits for the operation as we 
went from 8 operations to 10 with “delete obj.name” and “delete obj[index]". 
This again halved the number of program points available to 131072, but that's 
fortunately still about 10x what is really needed, so we should be good.

Thanks,
  Attila.