+1. Glad to have the WeakCacheValue! As an infinitesimally small nitpick, you could move removeClearedEntries call into findValue, but it’s really nothing; if you decide to do it, don’t bother posting a separate webrev for it :-)
Attila. > On Nov 11, 2015, at 1:14 PM, Hannes Wallnoefer <hannes.wallnoe...@oracle.com> > 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 >
