Review request for JDK-8134609: Allow constructors with same prototoype map to share the allocator map
Attila Szegedi
attila.szegedi at oracle.com
Mon Sep 14 09:48:32 UTC 2015
Overall, great work. Really well thought out; adding features to runtimes to optimize special cases always has the possibility of becoming complicated, and you have managed to keep the changes to the minimum, and pretty well contained. E.g. I like how even before your changes we have a fast path in ScriptObject.findProperty for the case where the property is not in the prototype, so the shared proto check there only kicks in when we do actually need to deal with prototypes, etc.
I only have some very small remarks:
- AllocatorMap could use getSharedPrototypeMap() in various places where it explicitly uses allocatorMap.getSharedProtoMap() now
- Debug.scriptStack() doesn't seem to be used
- PropertyMap.invalidateProtoGetSwitchPoint should be renamed invalidateProtoSwitchPoint. invalidateAllProtoGetSwitchPoints too
- PropertyMap.invalidateSharedProtoSwitchPoint: there's the "switchPoint != null && !switchPoint.hasBeenInvalidated()" predicate. Is it possible for switchPoint to not be null and be invalidated? If not, maybe hasBeenInvalidated should be an assert within if instead
- I wonder if "sharedProtoMap != myProto.getMap() || !myProto.getMap().isValidSharedProtoMap()" in ScriptObject.checkSharedProtoMap and a similar predicate "allocatorMap.getSharedProtoMap() != prototype.getMap() || !prototype.getMap().isValidSharedProtoMap())” in ScriptFunction.getAllocatorMap() couldn’t be extracted into a single method somewhere, if they share semantic similarity (syntactically they look very similar)
Attila.
> On Sep 11, 2015, at 2:20 PM, Hannes Wallnoefer <hannes.wallnoefer at oracle.com> wrote:
>
> Please review JDK-8134609: Allow constructors with same prototoype map to share the allocator map:
>
> http://cr.openjdk.java.net/~hannesw/8134609/webrev/
>
> The details for this are a bit tricky, so I added some notes to the jira bug:
>
> https://bugs.openjdk.java.net/browse/JDK-8134609?focusedCommentId=13843176&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13843176
>
> Thanks,
> Hannes
More information about the nashorn-dev
mailing list