akhudek / fast-zip

fast-zip
85 stars 16 forks source link

Add ClojureScript support #6

Closed noprompt closed 9 years ago

noprompt commented 10 years ago

I haven't run any benchmarks yet so I don't know if it's significantly faster than the vanilla clojure.zip for ClojureScript. Either way, this makes life easier for things like CLJX.

I had to change a couple of things to get the tests to pass in ClojureScript. (identical? :end :end) will return false in ClojureScript so = is used instead. And the .property getter syntax I changed to .-. That's pretty much all there was to it.

noprompt commented 10 years ago

I've done a bit of stress testing with this patch on a personal project and, as far as I can tell it all seems to "just work". Let me know what your thoughts are on this. I'm happy to maintain it as a personal fork but I'm sure others in the ClojureScript community would find value in this patch.

akhudek commented 10 years ago

Awesome, thanks. I will take a closer look at this tomorrow.

noprompt commented 10 years ago

Thanks. Let me know if you have any questions.

akhudek commented 10 years ago

I'm happy to merge this, but I think it really does need a benchmark. A library that purports to be faster than another should make some attempt to back up that claim. ClojureScript is a different enough target that benchmark results from Clojure may or may not translate to ClojureScript.

noprompt commented 10 years ago

@akhudek Certainly. OTOH I don't necessarily think that speed alone is a motivating reason to use this library over the one that ships with Clojure. The datatypes are another selling point. Still, if the benchmarks are a prerequisite for this patch to be merged, I'm happy to go the extra mile and uphold that end of it.

noprompt commented 9 years ago

@akhudek Sorry for the lag on getting back to you about the benchmarks. Life happens. :smile:

I came across a benchmarking library for JavaScript that can verify fast-zip's performance claims for ClojureScript. Following the example from the website's homepage I was able to come up with

(def suite
  (.. (.Suite Benchmark)
    (add ":clojure.zip" zip-test1)
    (add ":fast-zip" zip-test2)
    (on "cycle"
      (fn [e]
        (.log js/console (js/String (.-target e)))))
    (on "complete"
      (fn [e]
        (this-as *this*
          (let [fastest (-> *this*
                          (.filter "fastest")
                          (.pluck "name")
                          (aget 0))]
            (.log js/console "Fastest is" fastest)))))))

(.run suite)

which yielded the following results

:clojure.zip x 95.41 ops/sec ±1.48% (75 runs sampled)
:fast-zip x 216 ops/sec ±0.74% (87 runs sampled)
Fastest is :fast-zip

Though the output is different the results are forward. I tried tweaking the output to match up with what perforate displays but I couldn't figure out how to get it to match up word-for-word.

The next step for me will be to update the project file to include a build and a task for this (I've been hacking on it in the REPL).

Also, what are your thoughts about using generative testing for this project? I've used test.check to verify properties about zipper functions and it's a nice experience.

akhudek commented 9 years ago

Very nice! Glad to see the performance carries over.

I’m very in favour of generative testing. If you want to put some tests together I am more than happy to include them.

On September 16, 2014 at 1:48:45 AM, Joel Holdbrooks (notifications@github.com) wrote:

@akhudek Sorry for the lag on getting back to you about the benchmarks. Life happens.

I came across a benchmarking library for JavaScript that can verify fast-zip's performance claims for ClojureScript. Following the example from the website's homepage I was able to come up with

(def suite (.. (.Suite Benchmark) (add ":clojure.zip" zip-test1) (add ":fast-zip" zip-test2) (on "cycle" (fn [e](.log js/console %28js/String %28.-target e%29%29))) (on "complete" (fn [e](this-as this %28let [fastest %28-> this %28.filter) (.pluck "name") (aget 0))] (.log js/console "Fastest is" fastest)))))))

(.run suite) which yielded the following results

:clojure.zip x 95.41 ops/sec ±1.48% (75 runs sampled) :fast-zip x 216 ops/sec ±0.74% (87 runs sampled) Fastest is :fast-zip

Though the output is different the results are forward. I tried tweaking the output to match up with what perforate displays but I couldn't figure out how to get it to match up word-for-word.

The next step for me will be to update the project file to include a build and a task for this (I've been hacking on it in the REPL).

Also, what are your thoughts about using generative testing for this project? I've used test.check to verify properties about zipper functions and it's a nice experience.

— Reply to this email directly or view it on GitHub.

noprompt commented 9 years ago

OK. I've added the dependencies and code necessary to benchmark the ClojureScript code. It should be easy as lein clean-bench which should fetch the required Node dependences (thanks to lein-npm). The :notify-command for the "bench" build is set to run the benchmark on compile (which is nice for auto compiles as well).

akhudek commented 9 years ago

That’s very cool, thanks! I’m rather swamped with work at the moment but will try to test this and merge it later in the week.

On September 22, 2014 at 1:10:21 AM, Joel Holdbrooks (notifications@github.com) wrote:

OK. I've added the dependencies and code necessary to benchmark the ClojureScript code. It should be easy as lein clean-bench which should fetch the required Node dependences (thanks to lein-npm). The :notify-command for the "bench" build is set to run the benchmark on compile (which is nice for auto compiles as well).

— Reply to this email directly or view it on GitHub.

noprompt commented 9 years ago

Awesome. I would still like to add generative testing to this library at some point but I'll submit that as a separate request.

Thanks and let me know if there's anything else I can do.

ckirkendall commented 9 years ago

Any movement on this. :)