Skip to content

Commit

Permalink
CLJS-1483: Minor DCE regression with advanced compilation mode
Browse files Browse the repository at this point in the history
change :def case in compiler so that we once again only emit if init supplied.
The REPL is the only exception.

Without the above change foward declarations would prevent DCE.

fix minor DCE issue around murmur3 hashing.
  • Loading branch information
swannodette committed Nov 9, 2015
1 parent 30e302c commit 1346c3f
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 44 deletions.
36 changes: 23 additions & 13 deletions src/main/cljs/cljs/core.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -1198,8 +1198,13 @@
(next coll))
(mix-collection-hash hash-code n))))

(def ^:private empty-ordered-hash
(mix-collection-hash 1 0))
(def ^{:private true :jsdoc ["@type {*}"]}
EMPTY_ORDERED_HASH nil)

(defn- empty-ordered-hash []
(when (nil? EMPTY_ORDERED_HASH)
(set! EMPTY_ORDERED_HASH (mix-collection-hash 1 0)))
EMPTY_ORDERED_HASH)

(defn ^number hash-unordered-coll
"Returns the hash code, consistent with =, for an external unordered
Expand All @@ -1213,8 +1218,13 @@
(recur (inc n) (bit-or (+ hash-code (hash (first coll))) 0) (next coll))
(mix-collection-hash hash-code n))))

(def ^:private empty-unordered-hash
(mix-collection-hash 0 0))
(def ^{:private true :jsdoc ["@type {*}"]}
EMPTY_UNORDERED_HASH nil)

(defn- empty-unordered-hash []
(when (nil? EMPTY_UNORDERED_HASH)
(set! EMPTY_UNORDERED_HASH (mix-collection-hash 0 0)))
EMPTY_UNORDERED_HASH)

;;;;;;;;;;;;;;;;;;; protocols on primitives ;;;;;;;;
(declare hash-map list equiv-sequential)
Expand Down Expand Up @@ -2801,7 +2811,7 @@ reduces them without incurring seq initialization"
false))

IHash
(-hash [coll] empty-ordered-hash)
(-hash [coll] (empty-ordered-hash))

ISeqable
(-seq [coll] nil)
Expand Down Expand Up @@ -4916,7 +4926,7 @@ reduces them without incurring seq initialization"
(set! (.-EMPTY-NODE PersistentVector) (VectorNode. nil (make-array 32)))

(set! (.-EMPTY PersistentVector)
(PersistentVector. nil 0 5 (.-EMPTY-NODE PersistentVector) (array) empty-ordered-hash))
(PersistentVector. nil 0 5 (.-EMPTY-NODE PersistentVector) (array) (empty-ordered-hash)))

(set! (.-fromArray PersistentVector)
(fn [xs ^boolean no-clone]
Expand Down Expand Up @@ -5466,7 +5476,7 @@ reduces them without incurring seq initialization"
ICounted
(-count [coll] count))

(set! (.-EMPTY PersistentQueue) (PersistentQueue. nil 0 nil [] empty-ordered-hash))
(set! (.-EMPTY PersistentQueue) (PersistentQueue. nil 0 nil [] (empty-ordered-hash)))

(es6-iterable PersistentQueue)

Expand Down Expand Up @@ -5643,7 +5653,7 @@ reduces them without incurring seq initialization"
(-as-transient [coll]
(transient (into (hash-map) coll))))

(set! (.-EMPTY ObjMap) (ObjMap. nil (array) (js-obj) 0 empty-unordered-hash))
(set! (.-EMPTY ObjMap) (ObjMap. nil (array) (js-obj) 0 (empty-unordered-hash)))

(set! (.-HASHMAP_THRESHOLD ObjMap) 8)

Expand Down Expand Up @@ -5995,7 +6005,7 @@ reduces them without incurring seq initialization"
(-as-transient [coll]
(TransientArrayMap. (js-obj) (alength arr) (aclone arr))))

(set! (.-EMPTY PersistentArrayMap) (PersistentArrayMap. nil 0 (array) empty-unordered-hash))
(set! (.-EMPTY PersistentArrayMap) (PersistentArrayMap. nil 0 (array) (empty-unordered-hash)))

(set! (.-HASHMAP-THRESHOLD PersistentArrayMap) 8)

Expand Down Expand Up @@ -6931,7 +6941,7 @@ reduces them without incurring seq initialization"
(-as-transient [coll]
(TransientHashMap. (js-obj) root cnt has-nil? nil-val)))

(set! (.-EMPTY PersistentHashMap) (PersistentHashMap. nil 0 nil false nil empty-unordered-hash))
(set! (.-EMPTY PersistentHashMap) (PersistentHashMap. nil 0 nil false nil (empty-unordered-hash)))

(set! (.-fromArray PersistentHashMap)
(fn [arr ^boolean no-clone]
Expand Down Expand Up @@ -7724,7 +7734,7 @@ reduces them without incurring seq initialization"

(-comparator [coll] comp))

(set! (.-EMPTY PersistentTreeMap) (PersistentTreeMap. compare nil 0 nil empty-unordered-hash))
(set! (.-EMPTY PersistentTreeMap) (PersistentTreeMap. compare nil 0 nil (empty-unordered-hash)))

(es6-iterable PersistentTreeMap)

Expand Down Expand Up @@ -8045,7 +8055,7 @@ reduces them without incurring seq initialization"
(-as-transient [coll] (TransientHashSet. (-as-transient hash-map))))

(set! (.-EMPTY PersistentHashSet)
(PersistentHashSet. nil (.-EMPTY PersistentArrayMap) empty-unordered-hash))
(PersistentHashSet. nil (.-EMPTY PersistentArrayMap) (empty-unordered-hash)))

(set! (.-fromArray PersistentHashSet)
(fn [items ^boolean no-clone]
Expand Down Expand Up @@ -8191,7 +8201,7 @@ reduces them without incurring seq initialization"
(-lookup coll k not-found)))

(set! (.-EMPTY PersistentTreeSet)
(PersistentTreeSet. nil (.-EMPTY PersistentTreeMap) empty-unordered-hash))
(PersistentTreeSet. nil (.-EMPTY PersistentTreeMap) (empty-unordered-hash)))

(es6-iterable PersistentTreeSet)

Expand Down
65 changes: 34 additions & 31 deletions src/main/clojure/cljs/compiler.cljc
Original file line number Diff line number Diff line change
Expand Up @@ -602,37 +602,40 @@

(defmethod emit* :def
[{:keys [name var init env doc jsdoc export test var-ast]}]
(let [mname (munge name)]
(emit-comment env doc (concat jsdoc (:jsdoc init)))
(when (:def-emits-var env)
(when (= :return (:context env))
(emitln "return ("))
(emitln "(function (){"))
(emits var)
(when init
(emits " = "
(if-let [define (get-define mname jsdoc)]
define
init)))
(when (:def-emits-var env)
(emitln "; return (")
(emits (merge
{:op :var-special
:env (assoc env :context :expr)}
var-ast))
(emitln ");})()")
(when (= :return (:context env))
(emitln ")")))
;; NOTE: JavaScriptCore does not like this under advanced compilation
;; this change was primarily for REPL interactions - David
;(emits " = (typeof " mname " != 'undefined') ? " mname " : undefined")
(when-not (= :expr (:context env)) (emitln ";"))
(when export
(emitln "goog.exportSymbol('" (munge export) "', " mname ");"))
(when (and ana/*load-tests* test)
(when (= :expr (:context env))
(emitln ";"))
(emitln var ".cljs$lang$test = " test ";"))))
;; We only want to emit if an init is supplied, this is to avoid dead code
;; elimination issues. The REPL is the exception to this rule.
(when (or init (:def-emits-var env))
(let [mname (munge name)]
(emit-comment env doc (concat jsdoc (:jsdoc init)))
(when (:def-emits-var env)
(when (= :return (:context env))
(emitln "return ("))
(emitln "(function (){"))
(emits var)
(when init
(emits " = "
(if-let [define (get-define mname jsdoc)]
define
init)))
(when (:def-emits-var env)
(emitln "; return (")
(emits (merge
{:op :var-special
:env (assoc env :context :expr)}
var-ast))
(emitln ");})()")
(when (= :return (:context env))
(emitln ")")))
;; NOTE: JavaScriptCore does not like this under advanced compilation
;; this change was primarily for REPL interactions - David
;(emits " = (typeof " mname " != 'undefined') ? " mname " : undefined")
(when-not (= :expr (:context env)) (emitln ";"))

This comment has been minimized.

Copy link
@princejwesley

princejwesley Mar 24, 2016

@swannodette why do we have to prevent emitting ; for :expr?

If we have two expressions as below,

(def e 0)
(if (= e 0)
  1 2)

We will end up getting this error:

ERROR - (intermediate value)(intermediate value)(...) is not a function

I guess, this issue has to be addressed in (eval-str*, compile-str*)

(when export
(emitln "goog.exportSymbol('" (munge export) "', " mname ");"))
(when (and ana/*load-tests* test)
(when (= :expr (:context env))
(emitln ";"))
(emitln var ".cljs$lang$test = " test ";")))))

(defn emit-apply-to
[{:keys [name params env]}]
Expand Down

0 comments on commit 1346c3f

Please sign in to comment.