Review request for JDK-8134609: Allow constructors with same prototoype map to share the allocator map

[Hannes Wallnoefer]

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.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