Welcome! Please see the About page for a little more info on how this works.

0 votes
in ClojureScript by
The CLJS compiler will generate a lot of unnecessary function wrappers for functions created inside a {{let}} (and in certain cases outside). These function wrappers are only necessary when inside a {{loop}} or when the function uses {{recur}} to work arround quirks in JS {{var}}. As far as I can tell this commit [1] changed the behavior to instead always do it.

This can generate quite a lot of "extra" code that the Closure Compiler will not eliminate.

A dummy example shows that these wrapper fns can get quite excessive argument lists even if they don't use any of the locals that function wrapper is meant to preserve.

(ns test.app)

(defn other [x])

(defn dummy [{:strs [foo bar coll] :as p}]
  (let [x "test"
        y "test"
        z "test"

        a (fn [i] i)
        b (fn [i] i)
        c (fn [i] i)]

    (fn return []
      (array a b c))
    ))


The generated code

// Compiled by ClojureScript 1.10.520 {}
goog.provide('test.app');
goog.require('cljs.core');
test.app.other = function test$app$other(x) {
    return null;
};
test.app.dummy = function test$app$dummy(p__526) {
    var map__527 = p__526;
    var map__527__$1 = (!(map__527 == null)
    ? map__527.cljs$lang$protocol_mask$partition0$ & 64 ||
      cljs.core.PROTOCOL_SENTINEL === map__527.cljs$core$ISeq$
        ? true
        : false
    : false)
        ? cljs.core.apply.call(null, cljs.core.hash_map, map__527)
        : map__527;
    var p = map__527__$1;
    var foo = cljs.core.get.call(null, map__527__$1, 'foo');
    var bar = cljs.core.get.call(null, map__527__$1, 'bar');
    var coll = cljs.core.get.call(null, map__527__$1, 'coll');
    var x = 'test';
    var y = 'test';
    var z = 'test';
        // this could just be var a = function(i) { return i };
    var a = (function(x, y, z, map__527, map__527__$1, p, foo, bar, coll) {
        return function(i) {
            return i;
        };
    })(x, y, z, map__527, map__527__$1, p, foo, bar, coll);
    var b = (function(x, y, z, a, map__527, map__527__$1, p, foo, bar, coll) {
        return function(i) {
            return i;
        };
    })(x, y, z, a, map__527, map__527__$1, p, foo, bar, coll);
    var c = (function(x, y, z, a, b, map__527, map__527__$1, p, foo, bar, coll) {
        return function(i) {
            return i;
        };
    })(x, y, z, a, b, map__527, map__527__$1, p, foo, bar, coll);
    return (function(x, y, z, a, b,    c, map__527, map__527__$1, p, foo, bar, coll) {
        return function test$app$dummy_$_return() {
            return [a, b, c];
        };
    })(x, y, z, a, b, c, map__527, map__527__$1, p, foo, bar, coll);
};

//# sourceMappingURL=app.js.map


I didn't do any benchmarks yet but I'm pretty sure this has a certain runtime overhead as well.

The code that causes this should be optimized to only emit those wrappers when actually needed and also only for the locals that were actually used.

Alternatively we could use JS {{const}} (or {{let}}) since nowadays >98% of Browsers in use support this [2] and the Closure Compiler will generated those function wrappers for us with certain {{:language-out}} settings (ie. the current default would).
 

[1] https://github.com/clojure/clojurescript/commit/78d20eebbbad17d476fdce04f2afd7489a507df7
[2] https://caniuse.com/#feat=const

14 Answers

0 votes
by

Comment made by: thheller

Just adding another data point here from an actual project that uses a macro to generate some functions to be used later.

Optimized gibberish looks like this

`
ii(
ia,
Y,
(function() {

return function(X, m) {
  var r = ji(oh);
  m = Bh(m[0], X);
  li(X, r, Ng, null, "odd");
  mi(r, m);
  return [[r], [r, m]];
};

})(na, Y, ia, G, K, O, T, l, n, p, q, v, x, D),
(function() {

return function(X, m, r, t) {
  return ni(X, m, 1, r[0], t[0]);
};

})(na, Y, ia, G, K, O, T, l, n, p, q, v, x, D)
);
`

which is 403 bytes and could be 233 bytes instead if those wrappers are removed. We can see that Closure already removed the function bindings but leaves the IIFE itself intact, so we can absolutely nothing from those wrappers.

`
ii(
ia,
Y,
function(X, m) {

var r = ji(oh);
m = Bh(m[0], X);
li(X, r, Ng, null, "odd");
mi(r, m);
return [[r], [r, m]];

},
function(X, m, r, t) {

return ni(X, m, 1, r[0], t[0]);

}
);
`

I suspect that this can actually add up to quite a bit of code in real apps.

I'll see if can tackle this next week.

0 votes
by

Comment made by: dnolen

I like the idea of using JavaScript let for bindings and having Closure sort it out. This would remove a lot of custom code for handling this ourselves in ClojureScript.

0 votes
by
_Comment made by: thheller_

Since some of the comments where lost in the migration here is a quick recap.

The patch causes a build failure in cljs-canary for core.async and cljs-oops. cljs-oops can be ignored.

https://github.com/cljs-oss/canary/tree/results/reports/2019/05/17/job-000933-1.10.529-2c0eada6

I cannot figure out what is happening with core.async and looking at the code it seems to me that is should be crashing the node process always. It is a mystery to me why it works in the REPL but I can reliably repeat the crashes with and without the patch when using a regular compile and running node manually.

In core.async master repo running against the CLJS master I get the failure

$ clojure -Sdeps '{:paths ["src/test/cljs" "src/main/clojure"] :deps {org.clojure/clojurescript {:git/url "https://github.com/clojure/clojurescript.git" :sha "3a0c07477ae781bf521bdc2b074ed7b783bb93f3"}}}' -m cljs.main -d out -re node -c cljs.core.async.test-runner

$ node out/main.js  
Testing cljs.core.async.pipeline-test

Testing cljs.core.async.buffer-tests

Testing cljs.core.async.timers-test

Testing cljs.core.async.tests

/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.js:3376
}})();
   ^
Error: Assert failed: This exception is expected
false
    at /mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.cljs:112:9
    at /mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.cljs:112:9
    at cljs$core$async$tests$state_machine__18890__auto____1 (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.js:3376:4)
    at cljs$core$async$tests$state_machine__18890__auto__ (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.js:3392:62)
    at cljs$core$async$impl$ioc_helpers$run_state_machine (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/impl/ioc_helpers.cljs:35:23)
    at cljs$core$async$impl$ioc_helpers$run_state_machine_wrapped (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/impl/ioc_helpers.cljs:39:6)
    at /mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.cljs:112:9
    at Immediate.cljs$core$async$impl$dispatch$process_messages (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/impl/dispatch.cljs:18:7)
    at runCallback (timers.js:705:18)
    at tryOnImmediate (timers.js:676:5)


Using my forked repo with the patch applied I get a different stack trace (expected) with the same failure

$ clojure -Sdeps '{:paths ["src/test/cljs" "src/main/clojure"] :deps {org.clojure/clojurescript {:git/url "https://github.com/thheller/clojurescript.git" :sha "b83dcb00546d570ce74fcf130a70a82326857323"}}}' -m cljs.main -d out -re node -c cljs.core.async.test-runner  

$ node out/main.js  

Testing cljs.core.async.pipeline-test

Testing cljs.core.async.buffer-tests

Testing cljs.core.async.timers-test

Testing cljs.core.async.tests

/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.cljs:130
        (go
        ^
Error: Assert failed: This exception is expected
false
    at switch__20288__auto__ (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.cljs:130:9)
    at /mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.cljs:130:9
    at cljs$core$async$tests$state_machine__20289__auto____1 (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.js:4103:4)
    at cljs$core$async$tests$state_machine__20289__auto__ (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.js:4119:62)
    at cljs$core$async$impl$ioc_helpers$run_state_machine (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/impl/ioc_helpers.cljs:35:23)
    at cljs$core$async$impl$ioc_helpers$run_state_machine_wrapped (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/impl/ioc_helpers.cljs:39:6)
    at /mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/tests.cljs:130:9
    at Immediate.cljs$core$async$impl$dispatch$process_messages (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/impl/dispatch.cljs:18:7)
    at runCallback (timers.js:705:18)
    at tryOnImmediate (timers.js:676:5)


Still trying to figure out why things don't fail at the REPL but I kinda think that my patch is not the real cause here.
0 votes
by
_Comment made by: mfikes_

Here is how I run it


clojure -Sdeps '{:paths ["src/test/cljs" "src/main/clojure"] :deps {org.clojure/clojurescript {:git/url "https://github.com/clojure/clojurescript.git" :sha "3a0c07477ae781bf521bdc2b074ed7b783bb93f3"}}}' -m cljs.main -d out -re node src/test/cljs/cljs/core/async/test_runner.cljs


And the output is:


$ clojure -Sdeps '{:paths ["src/test/cljs" "src/main/clojure"] :deps {org.clojure/clojurescript {:git/url "https://github.com/clojure/clojurescript.git" :sha "3a0c07477ae781bf521bdc2b074ed7b783bb93f3"}}}' -m cljs.main -d out -re node src/test/cljs/cljs/core/async/test_runner.cljs

Testing cljs.core.async.pipeline-test

Testing cljs.core.async.buffer-tests

Testing cljs.core.async.timers-test

Testing cljs.core.async.tests
Error: Assert failed: This exception is expected
false
    at /Users/mfikes/Projects/core.async/out/cljs/core/async/tests.js:3318:19
    at /Users/mfikes/Projects/core.async/out/cljs/core/async/tests.js:3355:51
    at cljs$core$async$tests$state_machine__18881__auto____1 (/Users/mfikes/Projects/core.async/out/cljs/core/async/tests.js:3376:4)
    at cljs$core$async$tests$state_machine__18881__auto__ (/Users/mfikes/Projects/core.async/out/cljs/core/async/tests.js:3392:62)
    at cljs$core$async$impl$ioc_helpers$run_state_machine (/Users/mfikes/Projects/core.async/out/cljs/core/async/impl/ioc_helpers.js:96:74)
    at cljs$core$async$impl$ioc_helpers$run_state_machine_wrapped (/Users/mfikes/Projects/core.async/out/cljs/core/async/impl/ioc_helpers.js:99:63)
    at /Users/mfikes/Projects/core.async/out/cljs/core/async/tests.js:3407:67
    at Immediate.cljs$core$async$impl$dispatch$process_messages (/Users/mfikes/Projects/core.async/out/cljs/core/async/impl/dispatch.js:20:9)
    at processImmediate (internal/timers.js:443:21)
    at process.topLevelDomainCallback (domain.js:136:23)
Error: Assert failed: This exception is expected
false
    at /Users/mfikes/Projects/core.async/out/cljs/core/async/tests.js:3923:19
    at /Users/mfikes/Projects/core.async/out/cljs/core/async/tests.js:3961:51
    at cljs$core$async$tests$state_machine__18881__auto____1 (/Users/mfikes/Projects/core.async/out/cljs/core/async/tests.js:3982:4)
    at cljs$core$async$tests$state_machine__18881__auto__ (/Users/mfikes/Projects/core.async/out/cljs/core/async/tests.js:3998:62)
    at cljs$core$async$impl$ioc_helpers$run_state_machine (/Users/mfikes/Projects/core.async/out/cljs/core/async/impl/ioc_helpers.js:96:74)
    at cljs$core$async$impl$ioc_helpers$run_state_machine_wrapped (/Users/mfikes/Projects/core.async/out/cljs/core/async/impl/ioc_helpers.js:99:63)
    at /Users/mfikes/Projects/core.async/out/cljs/core/async.js:4290:67
    at /Users/mfikes/Projects/core.async/out/cljs/core/async.js:609:13
    at /Users/mfikes/Projects/core.async/out/cljs/core/async/impl/channels.js:201:12
    at Immediate.cljs$core$async$impl$dispatch$process_messages (/Users/mfikes/Projects/core.async/out/cljs/core/async/impl/dispatch.js:20:9)

Ran 44 tests containing 200 assertions.
0 failures, 0 errors.
0 votes
by

Comment made by: thheller

(link: ~mfikes): Yes, that is what I meant by running things in the REPL. Your way works without the patch but not with it. Running the node process separate from the compilation however fails in both instances. As far as I understand your way is running the code through a managed node-repl process.

I cannot figure out why your way works so I tried running things separately so I can run the process with {{node --inspect-brk out/main.js}} and attach a debugger. Since it always fails that way however I think that my patch is not actually at fault here. Debugging async code like this is hard ...

0 votes
by

Comment made by: mfikes

(link: ~thheller) Maybe it only works when in the REPL owing to the delayed Node shutdown code present in CLJS-2780.

0 votes
by

Comment made by: thheller

My understanding of {{node}} is that any uncaught Exception should cause the node process to (link: https://nodejs.org/api/process.html#process_event_uncaughtexception text: exit). This seems to be prevented in the REPL since the node-repl.js code uses to "domain" package (which is deprecated btw) to (link: https://github.com/clojure/clojurescript/blob/59997385d85e7e1af1559d599eb51fdb1d7e93b1/src/main/clojure/cljs/repl/node_repl.js#L50 text: trap) this error and thus prevent the exit.

What I can't figure out why this doesn't work with my patch.

In your stacktrace it starts in domain.js, with mine it doesn't. Can't figure out why since the code responsible for this isn't even CLJS compiled code so it really shouldn't be any different.

`
;; without patch

at Immediate.cljs$core$async$impl$dispatch$process_messages (/Users/mfikes/Projects/core.async/out/cljs/core/async/impl/dispatch.js:20:9)
at processImmediate (internal/timers.js:443:21)
at process.topLevelDomainCallback (domain.js:136:23)

;; with patch

at Immediate.cljs$core$async$impl$dispatch$process_messages (/mnt/c/Users/thheller/code/oss/core.async/out/cljs/core/async/impl/dispatch.cljs:18:7)
at runCallback (timers.js:705:18)
at tryOnImmediate (timers.js:676:5)

`

Either way this suggests that the patch isn't actually responsible for the failures, given that it fails regardless when running without that REPL error trap.

0 votes
by

Comment made by: mfikes

If you update {{core.async}}'s deps in its {{project.clj}}

  • clojure 1.10.0
  • lein-cljsbuild 1.1.7

and then run the unit tests as described in {{core.async}}'s README (which involves running them in a browser) they pass with ClojureScript 1.10.520 but never complete (showing the count of passing / failing tests) with ClojureScript master built with CLJS-3077.patch.

0 votes
by

Comment made by: thheller

Thanks Mike. I finally figured it out ... the logic assumed that all let bindings outside a loop do not need to be captured. The logic assumed however that is would be easy to identify when you are actually in a loop, which it did by detecting the first loop binding. It did not account for loop without bindings ....

`
(defn foo [a b]
(let [x ...] ;; no capture

(loop [] ;; sneaky
  (let [y ...]  ;; y needs to be captured but wasn't
    (side-effect (fn [] (use x y)))
    (recur))))

`

0 votes
by

Comment made by: thheller

New patch should be more reliable (and passes core.async tests). I also added a few tests which are admittely a bit dirty but I couldn't come up with a cleaner way to test this.

0 votes
by

Comment made by: mfikes

CLJS-3077-2.patch passes CI and Canary (/)
(With expected {{cljs-oops}} failure owing to generated code change.)

0 votes
by

Comment made by: mfikes

CLJS-3077-2.patch added to Patch Tender (i)

0 votes
by

Comment made by: mfikes

CLJS-3077-2.patch no longer applies.

0 votes
by
Reference: https://clojure.atlassian.net/browse/CLJS-3077 (reported by thheller)
...