ClojureScript

Optimize code gen for empty queue

Details

  • Type: Enhancement Enhancement
  • Status: In Progress In Progress
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Patch:
    Code and Test

Description

If you have #queue [] 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

Activity

Hide
Colin Kahn added a comment -

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

Show
Colin Kahn added a comment - Here's a go at doing this one. Not sure if the tests are how they should be.
Hide
Mike Fikes added a comment -

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

Show
Mike Fikes added a comment - 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
Hide
Colin Kahn added a comment -

Hi Mike,

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

Show
Colin Kahn added a comment - Hi Mike, Just did this morning, got the confirmation in my email.
Hide
Mike Fikes added a comment -

Great, I'll review the patch.

Show
Mike Fikes added a comment - Great, I'll review the patch.
Hide
Mike Fikes added a comment -

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.

Show
Mike Fikes added a comment - 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.
Hide
Colin Kahn added a comment -

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 [])
Show
Colin Kahn added a comment - 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 [])
Hide
Mike Fikes added a comment -

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.

Show
Mike Fikes added a comment - 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.
Hide
Colin Kahn added a comment - - edited

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.

Show
Colin Kahn added a comment - - edited 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.
Hide
Mike Fikes added a comment -

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

Show
Mike Fikes added a comment - 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
Hide
Mike Fikes added a comment -

With the patch #queue [] is free.

Benchmarking: (simple-benchmark [f (fn [] #queue [])] (f) 1e8)

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
Show
Mike Fikes added a comment - With the patch #queue [] is free. Benchmarking: (simple-benchmark [f (fn [] #queue [])] (f) 1e8) 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
Hide
Mike Fikes added a comment -

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.

Show
Mike Fikes added a comment - 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.
Hide
Mike Fikes added a comment -

CLJS-2916.patch added to Patch Tender

Show
Mike Fikes added a comment - CLJS-2916.patch added to Patch Tender

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated: