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

[Sundararajan Athijegannathan]

+1

NIce cleanup of PropertyMap as well!

-Sundar

On 9/16/2015 2:18 PM, Hannes Wallnoefer 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.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
>