ClojureScript

Add multi-arg versions of conj! disj! assoc! and dissoc!

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code

Description

The transient collection manipulation functions conj! disj! assoc! and dissoc! don't have an "& rest" argument form, so it is not possible to write for example (conj! t-vec :a :b).

Besides being inconvenient, this is also different from Clojure.

Patch attached.

  1. cljs-734-2.patch
    04/Jan/14 2:02 AM
    2 kB
    Francis Avila
  2. bench-variadic.txt
    04/Jan/14 2:02 AM
    2 kB
    Francis Avila
  3. bench-cljs-734.txt
    04/Jan/14 2:02 AM
    2 kB
    Francis Avila
  4. bench-master.txt
    04/Jan/14 2:02 AM
    2 kB
    Francis Avila
  5. benchmark_runner.cljs
    04/Jan/14 2:02 AM
    3 kB
    Francis Avila

Activity

Hide
David Nolen added a comment -

Please include a properly formatted squashed patch that includes tests -https://github.com/clojure/clojurescript/wiki/Patches. Thanks!

Show
David Nolen added a comment - Please include a properly formatted squashed patch that includes tests -https://github.com/clojure/clojurescript/wiki/Patches. Thanks!
Hide
Francis Avila added a comment -

Patch amended, tests added.

Show
Francis Avila added a comment - Patch amended, tests added.
Hide
Jozef Wagner added a comment -

Let's step back a little. Transient variants of conj, dissoc, etc. were created just for performance, not convenience. Clojure version of conj! DOES NOT support multi arg version and IMO it should not, as the transient variants should do one thing and do it fast, and not fiddle with seqs and deconstruct... I would go as far as to propose to remove multi arg versions from Clojure, rather than adding them in CLJS.

Show
Jozef Wagner added a comment - Let's step back a little. Transient variants of conj, dissoc, etc. were created just for performance, not convenience. Clojure version of conj! DOES NOT support multi arg version and IMO it should not, as the transient variants should do one thing and do it fast, and not fiddle with seqs and deconstruct... I would go as far as to propose to remove multi arg versions from Clojure, rather than adding them in CLJS.
Hide
Francis Avila added a comment - - edited

I didn't realize conj! wasn't variadic in Clojure--assoc! dissoc! and disj! are though. I would rather patch Clojure's conj! to add a variadic form.

I think we should support variadic forms of these functions because their non-transient counterparts do. Code typically starts out non-transient and then becomes transient later. If conj/conj! assoc/assoc! etc support the same function signatures then getting a working transient version is trivial--wrap the initial structure with transient, the result with persistent!, and add bangs to your conj/disj/assoc/dissoc names. It's an unnecessary annoyance to not have a variadic form in this case. This is precisely the annoying scenario that I've encountered enough times to drive me to write this patch.

We lose nothing by having a variadic form--if you want to avoid messing with seqs in your transient code you still can by calling the non-variadic form. But if you're using transients directly you're almost certainly have a seq somewhere in your code path anyway, so what's the harm in letting conj!/disj! etc consume it from the inside instead of forcing the user to do it on the outside.

Show
Francis Avila added a comment - - edited I didn't realize conj! wasn't variadic in Clojure--assoc! dissoc! and disj! are though. I would rather patch Clojure's conj! to add a variadic form. I think we should support variadic forms of these functions because their non-transient counterparts do. Code typically starts out non-transient and then becomes transient later. If conj/conj! assoc/assoc! etc support the same function signatures then getting a working transient version is trivial--wrap the initial structure with transient, the result with persistent!, and add bangs to your conj/disj/assoc/dissoc names. It's an unnecessary annoyance to not have a variadic form in this case. This is precisely the annoying scenario that I've encountered enough times to drive me to write this patch. We lose nothing by having a variadic form--if you want to avoid messing with seqs in your transient code you still can by calling the non-variadic form. But if you're using transients directly you're almost certainly have a seq somewhere in your code path anyway, so what's the harm in letting conj!/disj! etc consume it from the inside instead of forcing the user to do it on the outside.
Hide
David Nolen added a comment -

It would be nice to see actual performance numbers. If the performance difference is minor under V8 and JavaScriptCore I'm inclined to merge in this simple enhancement and consistency patch.

Show
David Nolen added a comment - It would be nice to see actual performance numbers. If the performance difference is minor under V8 and JavaScriptCore I'm inclined to merge in this simple enhancement and consistency patch.
Hide
Francis Avila added a comment - - edited
  • Patch updated:
    • Made variadic assoc! faster
    • rebased to latest master.
  • Wrote a benchmark, benchmark_runner.cljs, which I run using script/benchmark. Results on my machine attached: linux with x64 v8 and spidermonkey 17 (no JSCore benches).
    • On V8, there is no difference between master and my patch for non-variadic calls.
    • On spidermonkey, non-variadic vec assoc!, set disj!, and map dissoc! are slower with the patch; the rest are the same. I can't explain why!
    • The variadic calls (using apply) are in some cases faster than loop/recur with non-variadic calls, which I also can't understand.
Show
Francis Avila added a comment - - edited
  • Patch updated:
    • Made variadic assoc! faster
    • rebased to latest master.
  • Wrote a benchmark, benchmark_runner.cljs, which I run using script/benchmark. Results on my machine attached: linux with x64 v8 and spidermonkey 17 (no JSCore benches).
    • On V8, there is no difference between master and my patch for non-variadic calls.
    • On spidermonkey, non-variadic vec assoc!, set disj!, and map dissoc! are slower with the patch; the rest are the same. I can't explain why!
    • The variadic calls (using apply) are in some cases faster than loop/recur with non-variadic calls, which I also can't understand.
Hide
David Nolen added a comment -

Can we remove the old patches thanks.

Show
David Nolen added a comment - Can we remove the old patches thanks.
Hide
Francis Avila added a comment -

Actually I don't think I can, or at least I see no way to remove them in JIRA. The latest patch is cljs-734-2.patch

Show
Francis Avila added a comment - Actually I don't think I can, or at least I see no way to remove them in JIRA. The latest patch is cljs-734-2.patch
Hide
David Nolen added a comment -

K thanks, I've cleaned up the stale ones.

Show
David Nolen added a comment - K thanks, I've cleaned up the stale ones.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: