Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Comprehensive performance improvement to construction time of

PersistentArrayMap and PersistentHashSet
literals. cljs.core.PersistentArrayMap/fromArray now only takes on
array of KV pairs, and boolean flag no-clone. Compiler emits array map
literals with this set to true to avoid copy overhead.

PersistentArrayMap now calls through protocols on hash-map.

PersistentHashSet now starts with PersistentArrayMap underneath
instead of PersistentHashMap. PersistentHashSet now also takes an
array, if this array is less that threshold / 2 for
PersistentArrayMaps we just pass it along, again using the no-clone
trick. If larger than threshold we use transient PersistentHashSet.

Finally performance enhancement for hash-set. If we get an IndexedSeq,
which we will in the case of (hash-set 1 2 3) etc, we construct an
array and call cljs.core.PersistentHashSet/fromArray.
  • Loading branch information...
commit bd8c9af0fd85d99695c45f40f7e20da740395c23 1 parent c3a3129
David Nolen swannodette authored
9 benchmark/cljs/benchmark_runner.cljs
View
@@ -108,6 +108,7 @@
(println)
(println ";;; array-map")
+(simple-benchmark [] {[1] true [2] true [3] true} 1000000)
(simple-benchmark [coll (array-map)] (assoc coll :foo :bar) 1000000)
(simple-benchmark [coll (array-map :foo :bar)] (-lookup coll :foo) 1000000)
(simple-benchmark [coll (array-map :foo :bar)] (assoc coll :baz :woz) 1000000)
@@ -170,9 +171,11 @@
(println)
(println ";;; set ops")
-(simple-benchmark [] #{} 100000) ;; SLOW
-(simple-benchmark [] #{1 2 3} 100000) ;; SLOW
-(simple-benchmark [coll #{1 2 3}] (conj coll 4) 100000) ;; SLOW
+(simple-benchmark [] #{} 1000000)
+(simple-benchmark [] #{1 2 3} 1000000)
+(simple-benchmark [v [1 2 3]] (set v) 1000000)
+(simple-benchmark [] (hash-set 1 2 3) 1000000)
+(simple-benchmark [coll #{1 2 3}] (conj coll 4) 1000000)
(println)
(println ";;; seq ops")
10 src/clj/cljs/compiler.clj
View
@@ -252,11 +252,9 @@
"})")
(<= (count keys) array-map-threshold)
- (emits "cljs.core.PersistentArrayMap.fromArrays(["
- (comma-sep keys)
- "],["
- (comma-sep vals)
- "])")
+ (emits "cljs.core.PersistentArrayMap.fromArray(["
+ (comma-sep (interleave keys vals))
+ "], true)")
:else
(emits "cljs.core.PersistentHashMap.fromArrays(["
@@ -279,7 +277,7 @@
(if (empty? items)
(emits "cljs.core.PersistentHashSet.EMPTY")
(emits "cljs.core.PersistentHashSet.fromArray(["
- (comma-sep items) "])"))))
+ (comma-sep (interleave items (repeat "null"))) "], true)"))))
(defmethod emit :constant
[{:keys [form env]}]
94 src/cljs/cljs/core.cljs
View
@@ -3153,19 +3153,22 @@ reduces them without incurring seq initialization"
())))
(set! cljs.core.PersistentVector/EMPTY_NODE (pv-fresh-node nil))
-(set! cljs.core.PersistentVector/EMPTY (PersistentVector. nil 0 5 cljs.core.PersistentVector/EMPTY_NODE (array) 0))
+
+(set! cljs.core.PersistentVector/EMPTY
+ (PersistentVector. nil 0 5 cljs.core.PersistentVector/EMPTY_NODE (array) 0))
+
(set! cljs.core.PersistentVector/fromArray
- (fn [xs no-clone]
- (let [l (alength xs)
- xs (if (identical? no-clone true) xs (aclone xs))]
- (if (< l 32)
- (PersistentVector. nil l 5 cljs.core.PersistentVector/EMPTY_NODE xs nil)
- (let [node (.slice xs 0 32)
- v (PersistentVector. nil 32 5 cljs.core.PersistentVector/EMPTY_NODE node nil)]
- (loop [i 32 out (-as-transient v)]
- (if (< i l)
- (recur (inc i) (conj! out (aget xs i)))
- (persistent! out))))))))
+ (fn [xs ^boolean no-clone]
+ (let [l (alength xs)
+ xs (if no-clone xs (aclone xs))]
+ (if (< l 32)
+ (PersistentVector. nil l 5 cljs.core.PersistentVector/EMPTY_NODE xs nil)
+ (let [node (.slice xs 0 32)
+ v (PersistentVector. nil 32 5 cljs.core.PersistentVector/EMPTY_NODE node nil)]
+ (loop [i 32 out (-as-transient v)]
+ (if (< i l)
+ (recur (inc i) (conj! out (aget xs i)))
+ (persistent! out))))))))
(defn vec [coll]
(-persistent!
@@ -4059,14 +4062,11 @@ reduces them without incurring seq initialization"
(set! cljs.core.PersistentArrayMap/HASHMAP_THRESHOLD 8)
-(set! cljs.core.PersistentArrayMap/fromArrays
- (fn [ks vs]
- (let [len (count ks)]
- (loop [i 0
- out (transient cljs.core.PersistentArrayMap/EMPTY)]
- (if (< i len)
- (recur (inc i) (assoc! out (aget ks i) (aget vs i)))
- (persistent! out))))))
+(set! cljs.core.PersistentArrayMap/fromArray
+ (fn [arr ^boolean no-clone]
+ (let [arr (if no-clone arr (aclone arr))]
+ (let [cnt (/ (alength arr) 2)]
+ (PersistentArrayMap. nil cnt arr nil)))))
(declare array->transient-hash-map)
@@ -5738,7 +5738,7 @@ reduces them without incurring seq initialization"
ICollection
(-conj [coll o]
- (PersistentHashSet. meta (assoc hash-map o nil) nil))
+ (PersistentHashSet. meta (-assoc hash-map o nil) nil))
IEmptyableCollection
(-empty [coll] (with-meta cljs.core.PersistentHashSet/EMPTY meta))
@@ -5758,7 +5758,7 @@ reduces them without incurring seq initialization"
(-seq [coll] (keys hash-map))
ICounted
- (-count [coll] (count (seq coll)))
+ (-count [coll] (-count hash-map))
ILookup
(-lookup [coll v]
@@ -5770,7 +5770,7 @@ reduces them without incurring seq initialization"
ISet
(-disjoin [coll v]
- (PersistentHashSet. meta (dissoc hash-map v) nil))
+ (PersistentHashSet. meta (-dissoc hash-map v) nil))
IFn
(-invoke [coll k]
@@ -5779,18 +5779,23 @@ reduces them without incurring seq initialization"
(-lookup coll k not-found))
IEditableCollection
- (-as-transient [coll] (TransientHashSet. (transient hash-map))))
+ (-as-transient [coll] (TransientHashSet. (-as-transient hash-map))))
-(set! cljs.core.PersistentHashSet/EMPTY (PersistentHashSet. nil (hash-map) 0))
+(set! cljs.core.PersistentHashSet/EMPTY
+ (PersistentHashSet. nil cljs.core.PersistentArrayMap/EMPTY 0))
(set! cljs.core.PersistentHashSet/fromArray
- (fn [items]
- (let [len (count items)]
- (loop [i 0
- out (transient cljs.core.PersistentHashSet/EMPTY)]
- (if (< i len)
- (recur (inc i) (conj! out (aget items i)))
- (persistent! out))))))
+ (fn [items ^boolean no-clone]
+ (let [len (alength items)]
+ (if (<= (/ len 2) cljs.core.PersistentArrayMap/HASHMAP_THRESHOLD)
+ (let [arr (if no-clone items (aclone items))]
+ (PersistentHashSet. nil
+ (cljs.core.PersistentArrayMap/fromArray arr true) nil))
+ (loop [i 0
+ out (transient cljs.core.PersistentHashSet/EMPTY)]
+ (if (< i len)
+ (recur (+ i 2) (conj! out (aget items i)))
+ (persistent! out)))))))
(deftype TransientHashSet [^:mutable transient-map]
ITransientCollection
@@ -5902,12 +5907,25 @@ reduces them without incurring seq initialization"
(defn hash-set

This no longer creates only "hash" sets. Shouldn't there be an array-set function? We could rename this function to set* in the spirit of list its the signature variation list*.

Similar complaint applies to cljs.core.PersistentHashSet/fromArray.

Going slightly off topic: We need a generic factory function for maps too. Unfortunate that the word "map" is overloaded. Maybe make-map? Although people already use hash-map all over, maybe more apps would be better served by going the opposite direction and converting hash-map to be a generic factory function :-/

David Nolen Collaborator

Implementation detail to me. I see no need for array-set and I still question the existence of array-map. Not sure I see the rationale for generic map factory functions.

Honestly: It's only really useful for people like you and me so we can test particular implementations without having to do battle with internal heuristics. It's not really convenient to call constructors directly, since they often have extra fields.

Honestly, I wish we could push each data structure into it's own namespace and keep its helper functions and factory functions tucked away there. I'm all for taking array-map out of core. I just want some consistency between sets and maps and their backing implementations. JVM Clojure's data structures live in clojure.lang, maybe we should do something similar for ClojureScript?

David Nolen Collaborator

This sounds good in theory, but there's some serious intertwining right now between types, core fns, declare order, etc which would need some serious sorting out. One day ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
([] cljs.core.PersistentHashSet/EMPTY)
- ([& keys]
- (loop [in (seq keys)
- out (transient cljs.core.PersistentHashSet/EMPTY)]
- (if (seq in)
- (recur (next in) (conj! out (first in)))
- (persistent! out)))))
+ ([& ^not-native keys]
+ (if (and (instance? IndexedSeq keys)
+ (< (alength (.-arr keys)) cljs.core.PersistentArrayMap/HASHMAP_THRESHOLD))
+ (let [karr (.-arr keys)
+ klen (alength karr)
+ alen (* 2 klen)
+ arr (make-array alen)]
+ (loop [ki 0]
+ (if (< ki klen)
+ (let [ai (* 2 ki)]
+ (aset arr ai (aget karr ki))
+ (aset arr (inc ai) nil)
+ (recur (inc ki)))
+ (cljs.core.PersistentHashSet/fromArray arr true))))
+ (loop [in keys
+ ^not-native out (-as-transient cljs.core.PersistentHashSet/EMPTY)]
+ (if-not (nil? in)
+ (recur (-next in) (-conj! out (-first in)))
+ (-persistent! out))))))
(defn set
"Returns a set of the distinct elements of coll."
Brandon Bloom

This no longer creates only "hash" sets. Shouldn't there be an array-set function? We could rename this function to set* in the spirit of list its the signature variation list*.

Similar complaint applies to cljs.core.PersistentHashSet/fromArray.

Going slightly off topic: We need a generic factory function for maps too. Unfortunate that the word "map" is overloaded. Maybe make-map? Although people already use hash-map all over, maybe more apps would be better served by going the opposite direction and converting hash-map to be a generic factory function :-/

David Nolen

Implementation detail to me. I see no need for array-set and I still question the existence of array-map. Not sure I see the rationale for generic map factory functions.

Brandon Bloom

Honestly: It's only really useful for people like you and me so we can test particular implementations without having to do battle with internal heuristics. It's not really convenient to call constructors directly, since they often have extra fields.

Honestly, I wish we could push each data structure into it's own namespace and keep its helper functions and factory functions tucked away there. I'm all for taking array-map out of core. I just want some consistency between sets and maps and their backing implementations. JVM Clojure's data structures live in clojure.lang, maybe we should do something similar for ClojureScript?

David Nolen

This sounds good in theory, but there's some serious intertwining right now between types, core fns, declare order, etc which would need some serious sorting out. One day ...

Please sign in to comment.
Something went wrong with that request. Please try again.