Review request for JDK-8141702: Add support for Symbol property keys

[Sundararajan Athijegannathan]

+1

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:
>
> http://cr.openjdk.java.net/~hannesw/8141702/webrev.01/
>
> 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.
>
> Thanks,
> Hannes
>
> 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 
>> checking:
>>
>>     String name = ((Symbol)arg).getName();
>>     return globalSymbolRegistry.get(name) == arg ? name : 
>> Undefined.getUndefined();
>>
>> 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.
>>
>> Attila.
>>
>>> On Nov 10, 2015, at 4:35 PM, Sundararajan Athijegannathan 
>>> <sundararajan.athijegannathan at oracle.com> wrote:
>>>
>>> Hi,
>>>
>>> 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 
>>> of).
>>>
>>> That's it!
>>>
>>> +1
>>>
>>> -Sundar
>>>
>>> On 11/10/2015 8:12 PM, Hannes Wallnoefer wrote:
>>>> Please review JDK-8141702: Add support for Symbol property keys:
>>>>
>>>> http://cr.openjdk.java.net/~hannesw/8141702/
>>>>
>>>> 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.
>>>>
>>>> Thanks,
>>>> Hannes
>