Okay, this is indeed substantially even better than the first version. I, too, am much happier with it in its form.
Few very minor things: AllocatorMap.hasValidSharedProtoMap could use hasSharedProtoMap "Return number listeners added to a ScriptObject." - missing "of" "can shared" - missing "be" Other than these, +1. I’m fine if you fix these without a new webrev. Attila. > On Sep 16, 2015, at 10:48 AM, Hannes Wallnoefer > <hannes.wallnoe...@oracle.com> wrote: > > Thanks for the review! > > As discussed, in addition to the changes you suggested below I did some > refactoring on the PropertyMap class, making more fields final to better > reflect its immutable nature. I soon noticed that all the core fields (flags, > field/spill boundaries, class name) could quite easily be made final except > for the new shared map flags I introduced. That gave me an additional reason > to turn shared proto map feature into a separete PropertyMap subclass > (something I had already considered before). > > So I added the SharedPropertyMap subclass to represent the new shared > prototype property maps. All the things fell quite nicely into place, and I'm > much happier with the code now than with the first version. I also tweaked > the property listener code to make it a bit simpler. Finally, I discovered an > issue with unsetting prototypes with shared maps that was not handled > correctly in the first version which I fixed and added/improved tests for. > > Please review the new webrev: > > http://cr.openjdk.java.net/~hannesw/8134609/webrev.01/ > > Thanks, > Hannes > > Am 2015-09-14 um 11:48 schrieb Attila Szegedi: >> 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.wallnoe...@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 >
