ClojureScript

Missing IReduce implementations and typo in clojure.core.reducers/append!

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Environment:
    Fresh CLJS checkout from github
  • Patch:
    Code and Test

Description

1) IReduce protocol is missing implementation for array and List.
2) append! uses (.add array), while the correct array method name is push

Patch attached.

Note: what is your experience with array.push performance? ClojureScript reducers/foldcat uses arrays for append! / cat implementation. My experience was that (into [] ...) using conj! and TransientVector was at least 5 times faster than reducers/foldcat using array and push. The reason is probably inefficient push implementation in Chrome/V8

Activity

Hide
David Nolen added a comment -

Thanks for the patch. I doubt that array push is inefficient, but I imagine the regularity of TransientVector operations is something that V8 likes.

Show
David Nolen added a comment - Thanks for the patch. I doubt that array push is inefficient, but I imagine the regularity of TransientVector operations is something that V8 likes.
Hide
David Nolen added a comment -

BTW I notice you haven't submitted your Contributor Agreement (unless I'm mistaken). Please send that in at your earliest convenience thanks much!

Show
David Nolen added a comment - BTW I notice you haven't submitted your Contributor Agreement (unless I'm mistaken). Please send that in at your earliest convenience thanks much!
Hide
Daniel Skarda added a comment -

Patch for missing IReduce protocol for LazySeq

Show
Daniel Skarda added a comment - Patch for missing IReduce protocol for LazySeq
Hide
Daniel Skarda added a comment -

@swannodette: Ok, I will send it today.

Show
Daniel Skarda added a comment - @swannodette: Ok, I will send it today.
Hide
David Nolen added a comment -

Many thanks!

Show
David Nolen added a comment - Many thanks!

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: