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