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 <szegedia at gmail.com>:
> 
> Please review JDK-8193371 "Use Dynalink REMOVE operation in Nashorn" at <http://cr.openjdk.java.net/~attila/8193371/webrev.jdk> for <https://bugs.openjdk.java.net/browse/JDK-8193371>
> 
> 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.
>