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
