ClojureScript

A declare with :arglists should generate static function calls

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: 1.9.293
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Accepted

Description

This is performance enhancement.

  1. Problem
    After a declare the compiler doesn't know which arities the function will be defined with and hence generates code that checks if that arity is defined and then either calls it or uses a slower `xy-fn.call(null, ...)` construct. This is not optimal since it can be slower and also generates slightly more code.

Especially functions which only have one arity are problematic since they will end up being called with `xy-fn.call`.

  1. Affects
    Code that uses a function that was only declared and not def'ed yet. Such as `cons` in `IndexedSeq` or `conj!` in `TransientHashMap`.
    1. Performance
      A preliminary benchmark showed neglible improvements in Chrome v54 but a significant (factor of 2.2) performance benefit in Firefox.
  1. Solution
    Most of the declares should use `(def ^{:declare true, :arglists '([x xs])} cons)` and the compiler should take the `:arglists` into consideration and emit direct function calls instead.

Activity

Hide
A. R added a comment - - edited

Similarly, functions that call themselves recursively don't get invoked optimally. Such as:

  • push-tail
  • do-assoc
  • pop-tail
  • tv-push-tail
  • tv-pop-tail

Matters quite a bit for TreeMap kv-reduce + dissoc.

EDIT: Separately addressed: https://dev.clojure.org/jira/browse/CLJS-2038

Show
A. R added a comment - - edited Similarly, functions that call themselves recursively don't get invoked optimally. Such as:
  • push-tail
  • do-assoc
  • pop-tail
  • tv-push-tail
  • tv-pop-tail
Matters quite a bit for TreeMap kv-reduce + dissoc. EDIT: Separately addressed: https://dev.clojure.org/jira/browse/CLJS-2038
Hide
Mike Fikes added a comment -

Hey Andre, I'm looking into this, but if you look at the cons example, you can see the issue you describe with 1.9.293:

cljs.user=> *clojurescript-version*
"1.9.293"
cljs.user=> (.-cljs$core$ICollection$_conj$arity$2 (->IndexedSeq [] nil nil))
#object[Function "function (coll,o){
var self__ = this;
var coll__$1 = this;
return (cljs.core.cons.cljs$core$IFn$_invoke$arity$2 ? cljs.core.cons.cljs$core$IFn$_invoke$arity$2(o,coll__$1) : cljs.core.cons.call(null,o,coll__$1));
}"]

But, with 1.10.238 it is no longer there. I haven't yet discovered the reason.

cljs.user=> *clojurescript-version*
"1.10.238"
cljs.user=> (set! *print-fn-bodies* true)
true
cljs.user=> (.-cljs$core$ICollection$_conj$arity$2 (->IndexedSeq [] nil nil))
#object[Function "function (coll,o){
var self__ = this;
var coll__$1 = this;
return cljs.core.cons(o,coll__$1);
}"]
Show
Mike Fikes added a comment - Hey Andre, I'm looking into this, but if you look at the cons example, you can see the issue you describe with 1.9.293:
cljs.user=> *clojurescript-version*
"1.9.293"
cljs.user=> (.-cljs$core$ICollection$_conj$arity$2 (->IndexedSeq [] nil nil))
#object[Function "function (coll,o){
var self__ = this;
var coll__$1 = this;
return (cljs.core.cons.cljs$core$IFn$_invoke$arity$2 ? cljs.core.cons.cljs$core$IFn$_invoke$arity$2(o,coll__$1) : cljs.core.cons.call(null,o,coll__$1));
}"]
But, with 1.10.238 it is no longer there. I haven't yet discovered the reason.
cljs.user=> *clojurescript-version*
"1.10.238"
cljs.user=> (set! *print-fn-bodies* true)
true
cljs.user=> (.-cljs$core$ICollection$_conj$arity$2 (->IndexedSeq [] nil nil))
#object[Function "function (coll,o){
var self__ = this;
var coll__$1 = this;
return cljs.core.cons(o,coll__$1);
}"]
Hide
Thomas Heller added a comment -

IIRC this is related to https://dev.clojure.org/jira/browse/CLJS-1992

If a var is declare'd and used before the actual var is generated it generates non-optimal code.

Consider this example with declare

(declare foo)

(defn bar [a b]
  (foo a b))

(defn foo [a b]
  (+ a b))

;; bar is ineffecient
demo.browser.bar = (function demo$browser$bar(a,b){
return (demo.browser.foo.cljs$core$IFn$_invoke$arity$2 ? demo.browser.foo.cljs$core$IFn$_invoke$arity$2(a,b) : demo.browser.foo.call(null,a,b));
});
demo.browser.foo = (function demo$browser$foo(a,b){
return (a + b);
});

without declare and potentially with :arglists, the code generated for bar is more efficient

(defn foo [a b]
  (+ a b))

(defn bar [a b]
  (foo a b))

;; bar is now efficient
demo.browser.foo = (function demo$browser$foo(a,b){
return (a + b);
});
demo.browser.bar = (function demo$browser$bar(a,b){
return demo.browser.foo(a,b);
});
Show
Thomas Heller added a comment - IIRC this is related to https://dev.clojure.org/jira/browse/CLJS-1992 If a var is declare'd and used before the actual var is generated it generates non-optimal code. Consider this example with declare
(declare foo)

(defn bar [a b]
  (foo a b))

(defn foo [a b]
  (+ a b))

;; bar is ineffecient
demo.browser.bar = (function demo$browser$bar(a,b){
return (demo.browser.foo.cljs$core$IFn$_invoke$arity$2 ? demo.browser.foo.cljs$core$IFn$_invoke$arity$2(a,b) : demo.browser.foo.call(null,a,b));
});
demo.browser.foo = (function demo$browser$foo(a,b){
return (a + b);
});
without declare and potentially with :arglists, the code generated for bar is more efficient
(defn foo [a b]
  (+ a b))

(defn bar [a b]
  (foo a b))

;; bar is now efficient
demo.browser.foo = (function demo$browser$foo(a,b){
return (a + b);
});
demo.browser.bar = (function demo$browser$bar(a,b){
return demo.browser.foo(a,b);
});
Hide
A. R added a comment -

Mike, thanks for taking a look. Also make sure you have static fns on. I can reproduce Thomas's example. Though I agree that the examples that I listed in core are for some reason gone... Not sure why.

Show
A. R added a comment - Mike, thanks for taking a look. Also make sure you have static fns on. I can reproduce Thomas's example. Though I agree that the examples that I listed in core are for some reason gone... Not sure why.
Hide
Mike Fikes added a comment -

This behavior did indeed change with CLJS-1992.

What happens is that, when core.cljs is compiled, it is done within the scope of a with-core-cljs block.:

https://github.com/clojure/clojurescript/blob/adeaa9be63b7911d4ac0c7765c2ca8fd2aa4d507/src/main/clojure/cljs/compiler.cljc#L1454-L1458

So, in order to compile core.cljs, an initial pass is made to analyze core.cljs (due to with-core-cljs), followed by actual compilation. During compilation, each of the forms in core.cljs is analyzed (again, in this case), but, owing to CLJS-1992, any declare forms encountered during that second analyzis pass no longer eliminate ananlysis data accumulated during the first pass.

This effectively causes things to look like the following, where the second use of cons (during actual compilation) benefits from the analysis produced for (defn cons [] ,,,) in the first analysis pass:

;; Analysis

(declare cons)

(cons ,,,)

(defn cons [] ,,,)

;; Compilation

(declare cons)

(cons ,,,)

(defn cons [],,)

This explains the observed behavior, and implies that changes for this ticket may be limited only to code outside of core (assumig the existing analysis / compilation model remains in place). (We've already been effectively getting the desired benefit for core functions owing to CLJS-1992.)

Show
Mike Fikes added a comment - This behavior did indeed change with CLJS-1992. What happens is that, when core.cljs is compiled, it is done within the scope of a with-core-cljs block.: https://github.com/clojure/clojurescript/blob/adeaa9be63b7911d4ac0c7765c2ca8fd2aa4d507/src/main/clojure/cljs/compiler.cljc#L1454-L1458 So, in order to compile core.cljs, an initial pass is made to analyze core.cljs (due to with-core-cljs), followed by actual compilation. During compilation, each of the forms in core.cljs is analyzed (again, in this case), but, owing to CLJS-1992, any declare forms encountered during that second analyzis pass no longer eliminate ananlysis data accumulated during the first pass. This effectively causes things to look like the following, where the second use of cons (during actual compilation) benefits from the analysis produced for (defn cons [] ,,,) in the first analysis pass:
;; Analysis

(declare cons)

(cons ,,,)

(defn cons [] ,,,)

;; Compilation

(declare cons)

(cons ,,,)

(defn cons [],,)
This explains the observed behavior, and implies that changes for this ticket may be limited only to code outside of core (assumig the existing analysis / compilation model remains in place). (We've already been effectively getting the desired benefit for core functions owing to CLJS-1992.)
Hide
Mike Fikes added a comment - - edited

Hey Andre, without any changes it is possible to achieve this with 1.10.238, but with different meta (:fn-var and :method-params):

$ clj -m cljs.main -co '{:static-fns true}' -re node -r
ClojureScript 1.10.238
cljs.user=> (def ^:declared ^:fn-var ^{:method-params ([a b])} foo)
#'cljs.user/foo
cljs.user=> (defn bar [a b] (foo a b))
#'cljs.user/bar
cljs.user=> (set! *print-fn-bodies* true)
true
cljs.user=> bar
#object[cljs$user$bar "function cljs$user$bar(a,b){
return cljs.user.foo(a,b);
}"]

Is the proposal that, whatever ends up being done, this results in public API that ClojureScript users could employ? Or, is this just a "internal" consideration for namespaces that ship with the compiler?

If it is the former, is the proposal the following?

That

(declare ^{:arglists '([a b])} foo)

be an example of public use, with this perhaps being converted internally into a def that essentially makes use of private meta keys:

(def ^:declared ^:fn-var ^{:method-params ([a b])} foo)
Show
Mike Fikes added a comment - - edited Hey Andre, without any changes it is possible to achieve this with 1.10.238, but with different meta (:fn-var and :method-params):
$ clj -m cljs.main -co '{:static-fns true}' -re node -r
ClojureScript 1.10.238
cljs.user=> (def ^:declared ^:fn-var ^{:method-params ([a b])} foo)
#'cljs.user/foo
cljs.user=> (defn bar [a b] (foo a b))
#'cljs.user/bar
cljs.user=> (set! *print-fn-bodies* true)
true
cljs.user=> bar
#object[cljs$user$bar "function cljs$user$bar(a,b){
return cljs.user.foo(a,b);
}"]
Is the proposal that, whatever ends up being done, this results in public API that ClojureScript users could employ? Or, is this just a "internal" consideration for namespaces that ship with the compiler? If it is the former, is the proposal the following? That
(declare ^{:arglists '([a b])} foo)
be an example of public use, with this perhaps being converted internally into a def that essentially makes use of private meta keys:
(def ^:declared ^:fn-var ^{:method-params ([a b])} foo)
Hide
A. R added a comment -

IMO, we shouldn't leak the AST implementation details. I remember that David was ok with the syntax I initially proposed:

(declare ^{:arglists '([a b])} foo)

so this might be as easy as to add some rewriting into the {declare} macro. It's definitely something that other libraries might want to use (eg. datascript).

Thanks for the research on this. Looks like it should be a pretty easy change.

Only question to remain: What about error reporting if declare and a later {defn} don't match? Could lead to ugly runtime errors when a static dispatch is emitted to a particular arity but the {defn} was changed an is now a (eg) a single arity fn. Or maybe that's already covered?

Show
A. R added a comment - IMO, we shouldn't leak the AST implementation details. I remember that David was ok with the syntax I initially proposed:
(declare ^{:arglists '([a b])} foo)
so this might be as easy as to add some rewriting into the {declare} macro. It's definitely something that other libraries might want to use (eg. datascript). Thanks for the research on this. Looks like it should be a pretty easy change. Only question to remain: What about error reporting if declare and a later {defn} don't match? Could lead to ugly runtime errors when a static dispatch is emitted to a particular arity but the {defn} was changed an is now a (eg) a single arity fn. Or maybe that's already covered?
Hide
Mike Fikes added a comment -

Instead of putting it in the declare macro (which is currently reusing the one from Clojure, the attached patch just does a similar thing during analysis. It also adds a new warning that checks for declared vs. defined :arglists mismatches.

The patch also adds :arglists meta to all declare s in code that ships with the compiler, apart from those in cljs.core which we get for free owing to the double analysis described in a previous comment.

Feature documentation PR: https://github.com/clojure/clojurescript-site/pull/234

Demo:

$ clj -A:cljs/dev -co '{:static-fns true}' -r
cljs.user=> (declare ^{:arglists '([x y])} foo)
#'cljs.user/foo
cljs.user=> (defn arity-wrong [] (foo 1))
WARNING: Wrong number of args (1) passed to cljs.user/foo at line 1 <cljs repl>
#'cljs.user/arity-wrong
cljs.user=> (defn arity-right [] (foo 1 2))
#'cljs.user/arity-right
cljs.user=> (set! *print-fn-bodies* true)
true
cljs.user=> arity-right
#object[cljs$user$arity_right "function cljs$user$arity_right(){
return cljs.user.foo((1),(2));
}"]
cljs.user=> (defn foo [x y z])
WARNING: cljs.user/foo declared arglists ([x y]) mismatch defined arglists ([x y z]) at line 1 <cljs repl>
#'cljs.user/foo
cljs.user=> (declare ^{:arglists '([x])} bar)
#'cljs.user/bar
cljs.user=> (defn bar ([x] 1) ([x y] 2))
WARNING: cljs.user/bar declared arglists ([x]) mismatch defined arglists ([x] [x y]) at line 1 <cljs repl>
Show
Mike Fikes added a comment - Instead of putting it in the declare macro (which is currently reusing the one from Clojure, the attached patch just does a similar thing during analysis. It also adds a new warning that checks for declared vs. defined :arglists mismatches. The patch also adds :arglists meta to all declare s in code that ships with the compiler, apart from those in cljs.core which we get for free owing to the double analysis described in a previous comment. Feature documentation PR: https://github.com/clojure/clojurescript-site/pull/234 Demo:
$ clj -A:cljs/dev -co '{:static-fns true}' -r
cljs.user=> (declare ^{:arglists '([x y])} foo)
#'cljs.user/foo
cljs.user=> (defn arity-wrong [] (foo 1))
WARNING: Wrong number of args (1) passed to cljs.user/foo at line 1 <cljs repl>
#'cljs.user/arity-wrong
cljs.user=> (defn arity-right [] (foo 1 2))
#'cljs.user/arity-right
cljs.user=> (set! *print-fn-bodies* true)
true
cljs.user=> arity-right
#object[cljs$user$arity_right "function cljs$user$arity_right(){
return cljs.user.foo((1),(2));
}"]
cljs.user=> (defn foo [x y z])
WARNING: cljs.user/foo declared arglists ([x y]) mismatch defined arglists ([x y z]) at line 1 <cljs repl>
#'cljs.user/foo
cljs.user=> (declare ^{:arglists '([x])} bar)
#'cljs.user/bar
cljs.user=> (defn bar ([x] 1) ([x y] 2))
WARNING: cljs.user/bar declared arglists ([x]) mismatch defined arglists ([x] [x y]) at line 1 <cljs repl>
Hide
Thomas Heller added a comment -

This is fantastic.

Although I'd ask to include the arglists annotations in cljs.core as well where appropriate since shadow-cljs does not do the double-analyze. I'm happy to create a proper patch for those once this is merged. I don't think there are any high-priority cases left and a couple of declares are outdated anyways (e.g. into-array).

Show
Thomas Heller added a comment - This is fantastic. Although I'd ask to include the arglists annotations in cljs.core as well where appropriate since shadow-cljs does not do the double-analyze. I'm happy to create a proper patch for those once this is merged. I don't think there are any high-priority cases left and a couple of declares are outdated anyways (e.g. into-array).
Hide
Mike Fikes added a comment -

Thomas: Ticket to add annotations to core has been written: CLJS-2826

Show
Mike Fikes added a comment - Thomas: Ticket to add annotations to core has been written: CLJS-2826

People

Vote (7)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: