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