A declare with :arglists should generate static function calls

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.  

    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.

Environment

None

Activity

Show:
Mike Fikes
July 15, 2018, 11:43 PM

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

Thomas Heller
May 25, 2018, 9:50 AM

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).

Mike Fikes
May 25, 2018, 5:04 AM

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:

Andre R
May 24, 2018, 10:24 AM

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

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?

Completed

Assignee

David Nolen

Reporter

Andre R

Labels

Approval

Accepted

Patch

Code and Test

Affects versions

Priority

Major