From my tests I concluded that leaving the queue threshold high as to not affect ‚ordinary‘ objects is the way to go. But it certainly doesn’t hurt to make the value configurable. I uploaded a new webrev that does this with a system property called „nashorn.propmap.queue.threshold“. It’s otherwise unchanged.
http://cr.openjdk.java.net/~hannesw/8068513/webrev.03/ Hannes > Am 16.10.2017 um 07:43 schrieb Sundararajan Athijegannathan > <sundararajan.athijegannat...@oracle.com>: > > Is it worthwhile adding configurability for QUEUE_THREASHOLD? Not a per > context property/option - but even a simple nashorn.* system property perhaps? > > +1 other than that > > -Sundar > > On 13/10/17, 7:55 PM, Hannes Wallnöfer wrote: >> I uploaded a new webrev, please review: >> >> http://cr.openjdk.java.net/~hannesw/8068513/webrev.02/ >> >> Changes to previous webrev: >> >> - updated to consolidated repository layout >> - fixed typos and improved/added some comments >> - improved test >> >> Thanks, >> Hannes >> >> >>> Am 11.09.2017 um 16:18 schrieb Hannes >>> Wallnöfer<hannes.wallnoe...@oracle.com>: >>> >>> Unfortunately I rushed the first webrev a bit, and a couple of bugs slipped >>> in. >>> >>> - PropertyHashMap(MapBuilder) constructor checks its own bins field >>> instead of MapBuilder’s for calculating threshold >>> - ElementQueue.cloneAndMerge() updates the queue field in PropertyHashMap >>> instead of just returning cloned and merged bins >>> >>> I uploaded a new webrev that fixes these problems, everything I wrote in my >>> original RFR still applies. >>> >>> http://cr.openjdk.java.net/~hannesw/8068513/webrev.01/ >>> >>> Thanks, >>> Hannes >>> >>> >>>> Am 05.09.2017 um 19:57 schrieb Hannes >>>> Wallnöfer<hannes.wallnoe...@oracle.com>: >>>> >>>> Please review 8068513: Adding elements to a javascript 'object' (a map) is >>>> slow: >>>> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8068513 >>>> Webrev: http://cr.openjdk.java.net/~hannesw/8068513/webrev.00/ >>>> >>>> This adds a new singly linked list called ‚ElementQueue‘ to >>>> PropertyHashMap that is used above a certain map size to store newly >>>> inserted elements without having to hash them (and therefore clone the >>>> bins array) immediately. Instead, The queue is merged into the hash bins >>>> at certain intervals, either every 512th insertions, or when a map's queue >>>> is searched for properties more than a few times. >>>> >>>> Merging the queue every 512 insertions proved to be the best balance >>>> between keeping the list searchable (we still need to check it for >>>> duplicates when adding elements) and avoiding too frequent cloning. >>>> >>>> In order to merge the queue to optimise query performance, the queue field >>>> needs to be non-final. To preserve thread safety, ElementQueue bundles >>>> both the bins and queue components, so it can replace both with the update >>>> of a single reference in PropertyHashMap. The old and new ElementQueue >>>> instances logically contain the same elements, so it is safe for other >>>> threads to keep using the old instance. I was thinking of maybe making the >>>> queue field volatile, but I don’t think this should be an issue. >>>> >>>> As part of this change I also added a new MapBuilder class that helps >>>> derive new maps from the existing ones by adding, replacing, or removing >>>> elements. The code is a bit more complex now with three possible storage >>>> data structures (list, bins, queue), but it’s still not too bad. >>>> >>>> I made sure that the code used for maps beneath the queue threshold is >>>> largely the same as before. Performance of the new combined behaveior is >>>> very close to before. The queued implementation itself performs pretty >>>> close to the normal implementation (apart from insertion on large maps of >>>> course) - I tested much lower thresholds during development, and it was >>>> still very good. >>>> >>>> Of course, all tests pass and performance is comparable or maybe slightly >>>> faster for some code. >>>> >>>> Thanks, >>>> Hannes
