Review request for JDK-8141702: Add support for Symbol property keys
sundararajan.athijegannathan at oracle.com
Wed Nov 11 14:36:49 UTC 2015
On 11/11/2015 5:44 PM, Hannes Wallnoefer wrote:
> Thanks for the reviews! I uploaded a new webrev that includes most of
> your suggestions:
> A few notes:
> - I followed Attila's suggestion regarding
> NativeSymbol.globalSymbolRegistry, creating a new
> jdk.nashorn.internal.WeakValueCache class that is used for both
> anonymous classes in Context and interned symbols in NativeSymbol.
> - I didn't use ConcurrentHashTable instead of synchronization since I
> envision symbol interning to be an infrequent operation. We can
> revisit this later on I guess.
> - I didn't really find a good place for a comment about symbols and
> codegen/serialization. However, I decided to make Symbol serializable
> even though it does not have to be yet.
> Am 2015-11-10 um 18:18 schrieb Attila Szegedi:
>> Great work!
>> I have few remarks on the NativeSymbol.globalSymbolRegistry:
>> I think it’s okay to have a static String->Symbol map in
>> NativeSymbol.globalSymbolRegistry, but it’d need to be a Map<String,
>> SymbolReference> where SymbolReference is a "class SymbolReference
>> extends WeakReference<Symbol>" and also carries the string key as its
>> field. A reference queue would be needed to remove String keys for
>> which references were cleared. This should pretty much work exactly
>> the same as Context.anonymousHostClasses works. Maybe you could
>> extract that code into a utility class?
>> For keyFor, instead of containsValue (linear time lookup) how about
>> String name = ((Symbol)arg).getName();
>> return globalSymbolRegistry.get(name) == arg ? name :
>> instead? Again, if you’d adopt my suggestion for references, then
>> get() returns a reference instead of a symbol, but the principle is
>> the same.
>> Finally, globalSymbolRegistry is a HashMap, and you synchronize
>> around it. I’d consider using a ConcurrentHashMap instead and avoid
>> synchronization. There’s the minor issue that if you adopt my
>> reference suggestion above then you can’t atomically deal with the
>> situation where the reference is cleared. You can still avoid
>> synchronization using a ConcurrentMap and computeIfAbsent if you want
>> to avoid synchronization, but you’ll need to additionally validate
>> that a non-absent reference is not cleared, and if it is, then create
>> a new one and do an additional putIfAbsent. The interaction between
>> FOR and keyFor is not racy anyway, as in order to not get back
>> undefined, one already has to have a strong reference to a symbol
>> that’s in the map.
>>> On Nov 10, 2015, at 4:35 PM, Sundararajan Athijegannathan
>>> <sundararajan.athijegannathan at oracle.com> wrote:
>>> Nice work!
>>> Few comments:
>>> * the symbol "internalizing" table could be per Context. Keeping it
>>> as static field from that class => Symbol instances are leaked
>>> permanently. Since we have 1:1 between script engine and nashorn
>>> Context instance, it is better if we keep it as Context-wide global.
>>> That way, when the engine dies, it's Symbols can be GC'ed.
>>> * Classes like NativeSymbol, Symbol can be final.
>>> * There could be comment somewhere that codegenerator will take care
>>> of String or Number properties in places where 'serialize'
>>> Properties, PropertyMaps (i.e., arbitrary Object value is taken care
>>> That's it!
>>> On 11/10/2015 8:12 PM, Hannes Wallnoefer wrote:
>>>> Please review JDK-8141702: Add support for Symbol property keys:
>>>> This adds support for Symbols. Symbols are a new type of
>>>> guaranteed-unique property keys that can be used to add properties
>>>> to objects without risking conflicts with existing properties.
>>>> Symbols are used by many other features in ES6, so it helps a lot
>>>> to have this in.
>>>> The patch looks big but for most files it's just the type of
>>>> property keys that changed from String to Object. This change is
>>>> pretty simple, and it does not affect the fast property access path.
>>>> I had to jump through some hoops in order to deal with some of the
>>>> strange characteristics of symbols, such as the fact that implicit
>>>> conversion to string or number throws a TypeError. For example, I
>>>> introduced a dedicated toString method in tools.Shell to render
>>>> results so it could display symbols both as top level values and
>>>> when contained in an array (which neither JSType.toString nor
>>>> ScriptRuntime.safeToString would let us do).
>>>> I think I explained all these cases sufficiently, but feel free to
>>>> ask if something looks weird.
>>>> This change is completely invisible in ES5 mode. I also added a
>>>> test that checks the absence of this and other ES6 features in ES5
>>>> mode as suggested by Sundar.
More information about the nashorn-dev