Nice work indeed. +1
Hannes > Am 14.12.2017 um 12:42 schrieb Attila Szegedi <szege...@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. >
