<< Back to previous view

[CLJS-734] Add multi-arg versions of conj! disj! assoc! and dissoc! Created: 26/Dec/13  Updated: 23/Jan/14  Resolved: 23/Jan/14

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File bench-cljs-734.txt     File benchmark_runner.cljs     Text File bench-master.txt     Text File bench-variadic.txt     Text File cljs-734-2.patch    
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.



 Comments   
Comment by David Nolen [ 26/Dec/13 12:47 PM ]

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

Comment by Francis Avila [ 26/Dec/13 4:29 PM ]

Patch amended, tests added.

Comment by Jozef Wagner [ 28/Dec/13 1:36 PM ]

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.

Comment by Francis Avila [ 28/Dec/13 4:39 PM ]

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.

Comment by David Nolen [ 30/Dec/13 8:25 AM ]

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.

Comment by Francis Avila [ 04/Jan/14 2:02 AM ]
  • 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.
Comment by David Nolen [ 07/Jan/14 7:08 AM ]

Can we remove the old patches thanks.

Comment by Francis Avila [ 07/Jan/14 8:17 AM ]

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

Comment by David Nolen [ 07/Jan/14 8:19 AM ]

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

Comment by David Nolen [ 23/Jan/14 8:08 AM ]

fixed, https://github.com/clojure/clojurescript/commit/8b2ed1d57b8904341727d4b5b251c8815e618a10

Generated at Thu Nov 20 14:59:21 CST 2014 using JIRA 4.4#649-r158309.