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