Optimize code gen for empty queue

Question

If you have 1. } in your code, the code generated is

cljs.core.into.call(null,cljs.core.PersistentQueue.EMPTY,cljs.core.PersistentVector.EMPTY)

when it could, for the empty vector case generate

cljs.core.PersistentQueue.EMPTY

See https://github.com/clojure/clojurescript/blob/f289ffee2270567f7976d45012a0a52c38eb6488/src/main/clojure/cljs/tagged_literals.cljc#L21

Answer 1

Comment made by: colinkahn

Here's a go at doing this one. Not sure if the tests are how they should be.

Answer 2

Comment made by: mfikes

Hey Colin. Thanks for the contribution! Have you signed the CA? I don't see your name listed here https://clojure.org/community/contributors

If not, please see https://clojurescript.org/community/contributing

Answer 3

Comment made by: colinkahn

Hi Mike,

Just did this morning, got the confirmation in my email.

Answer 4

Comment made by: mfikes

Great, I'll review the patch.

Answer 5

Comment made by: mfikes

Hey Colin,

For your 2nd test, did you perhaps mean to check this?

(instance? PersistentQueue #queue [1 2 3])

In terms of actually testing the patch, the only thing I can think of is to inspect the generated code. Looking at the string produced by

(binding [*print-fn-bodies* true] (pr-str (fn [] #queue [])))

seems like a bit of a hack, but you could check if the string produced contains {{PersistentVector}} and if so, fail the test.

Answer 6

Comment made by: colinkahn

Yes, definitely meant to check the type. Oddly I didn't get a failure from that in the test report. I was curious if there's a way to run just a subset of the tests?

What do you think of using with-redefs?

`(with-redefs [into (fn [& _] (throw (ex-info "Inefficiently created PersistentQueue" {})))] #queue [])`

Answer 7

Comment made by: mfikes

Hi Colin,

I'm not aware of a way to just run a subset of the tests. FWIW, Travis CI fails the build https://travis-ci.org/mfikes/clojurescript/builds/430674720

I definitely like your {{with-redefs}} approach. Much more clever than my string hack. :)

Answer 8

Comment made by: colinkahn

Hi Mike,

I updated the attached patch.

Those tests don't actually get run when you do 'lein test', looks like Travis does a build and runs them using jsc. I replicated locally, not sure if there are commands to make that easier.

Answer 9

Comment made by: mfikes

Thanks. I'll take a look at the revised patch. These tests are run via {{script/test}}. More info at https://clojurescript.org/community/running-tests

Answer 10

Comment made by: mfikes

With the patch 1. is free.

Benchmarking: 1. }

Before:

Benchmarking with V8 [f (fn [] (cljs.core/into cljs.core.PersistentQueue.EMPTY []))], (f), 100000000 runs, 442 msecs Benchmarking with SpiderMonkey [f (fn [] (cljs.core/into cljs.core.PersistentQueue.EMPTY []))], (f), 100000000 runs, 988 msecs Benchmarking with JavaScriptCore [f (fn [] (cljs.core/into cljs.core.PersistentQueue.EMPTY []))], (f), 100000000 runs, 2041 msecs Benchmarking with Nashorn [f (fn [] (cljs.core/into cljs.core.PersistentQueue.EMPTY []))], (f), 100000000 runs, 47257 msecs Benchmarking with ChakraCore [f (fn [] (cljs.core/into cljs.core.PersistentQueue.EMPTY []))], (f), 100000000 runs, 3040 msecs Benchmarking with GraalVM [f (fn [] (cljs.core/into cljs.core.PersistentQueue.EMPTY []))], (f), 100000000 runs, 296 msecs

After:

Benchmarking with V8 [f (fn [] cljs.core.PersistentQueue.EMPTY)], (f), 100000000 runs, 0 msecs Benchmarking with SpiderMonkey [f (fn [] cljs.core.PersistentQueue.EMPTY)], (f), 100000000 runs, 0 msecs Benchmarking with JavaScriptCore [f (fn [] cljs.core.PersistentQueue.EMPTY)], (f), 100000000 runs, 0 msecs Benchmarking with Nashorn [f (fn [] cljs.core.PersistentQueue.EMPTY)], (f), 100000000 runs, -1 msecs Benchmarking with ChakraCore [f (fn [] cljs.core.PersistentQueue.EMPTY)], (f), 100000000 runs, -5 msecs Benchmarking with GraalVM [f (fn [] cljs.core.PersistentQueue.EMPTY)], (f), 100000000 runs, -1 msecs

Answer 11

Comment made by: mfikes

CLJS-2916.patch of 19/Sep/18 6:12 PM LGTM.

It passes all tests, including Canary tests. Perf looks good given the previous comment.

Answer 12

Comment made by: mfikes

CLJS-2916.patch added to Patch Tender (i)

Answer 13

Reference: https://clojure.atlassian.net/browse/CLJS-2916 (reported by mfikes)