cycle, iterate, repeat return vals should IReduceInit

Description

Screened

clj-1603-15.patch (Java approach)

Alternatives:

There were a number of possible approaches for these enhancements:
1) Straight Java impl. The benefit of this approach is improving the performance of both the seq and reduce paths at the expense of writing a bunch of Java. See clj-1603-15.patch for the best of these impls.

2) Clojure deftype. This required moving cycle and iterate and providing a repeat1 implementation until deftype is defined (similar to the approach with reduce1). The deftype version returns a Seqable, IReduce object and has effectively the same former implementation for seq with a new fast implementation for IReduce. This makes reduce paths fast, but leaves seq paths about the same, with the benefit of no new Java code. The downside was that the new seqs did not implement the full range of interfaces from prior impls, which could potentially break code. See clj-1603-12-2.patch for a patch that covers this.

3) Add Iterable or IReduceInit directly to LazySeq. Conceptually, this does not make sense for general lazy seqs. Seqs materialize and cache each value once and doing this along with the ability to iterate/reduce introduces issues with caching (might as well use seqs for that) and synchronization. However, doing this for just these specific lazy seqs is possible - see latest patch.

Approach: Latest patch makes LazySeq implement IReduce and internal macro reducible-lazy-seq lets callers supply a function to implement the reduce path.

A few things to note:

  • Added some example-based tests for iterate, cycle, and repeat where I thought they were needed. Did not add generative tests - not clear to me what these would be that would actually be valuable. All of these functions are pretty simple and the examples cover the special cases.

Performance:

Some example timing, all in µs:

Expression

1.6.0

1.7.0-alpha5

master + clj-1603-15 (Java)

master + clj-1603-12-2 (deftype)

master + clj-1603-14 (split)

(doall (take 1000 (repeat 1)))

87

93

63

89

92

(into [] (take 1000) (repeat 1))

n/a

67

25

27

33

(doall (repeat 1000 1))

87

94

16

94

89

(into [] (repeat 1000 1))

99

110

13

12

12

(reduce + 0 (repeat 1000 1))

99

126

20

22

25

(into [] (take 1000) (repeat 1))

n/a

67

28

33

27

(doall (take 1000 (cycle [1 2 3])))

101

106

85

108

103

(into [] (take 1000) (cycle [1 2 3]))

n/a

73

38

45

44

(doall (take 1000 (iterate inc 0)))

93

98

75

123

116

(into [] (take 1000) (iterate inc 0))

n/a

85

38

40

39

Notes on timings above:

  • All reduce timings (with into) comparable across the impls and significantly better than the current behavior over seqs.

  • The Java impl is faster across the board with doall. doall repeatedly calls seq() and next() to walk the sequence. The Java class versions of Repeat, Cycle, and Iterate extend ASeq and seq() just return this. next() constructs and returns a new instance of the class, which is immutable. In the lazy seq versions, LazySeq is mutable and requires synchronization and handling the caching safely. So, simple immutable instance ftw here.

  • The Java finite repeat has an extra benefit from using a primitive long for the counter.

  • One performance difference that's not visible in the timings is that the Java implementations have the benefit of being both seqs and reducibles as they are traversed, so you can always get a fast reduce. The deftype and split impls are only reducible at the initial instance, walking off that initial head reverts to lazy seqs that are not quickly reducible.

Patch: The two patches in leading contention are:

  • clj-1603-15.patch - for the Java version

  • clj-1603-14.patch - for the split impl

Alex opinion: I have swung back and forth on this but my current recommendation is for the Java implementation (clj-1603-15.patch). It's faster for both seqs and reduce, both in the timings above and importantly in maintaining reducibility as they are traversed. There is more Java, but I've made my peace with that - the code maximally leverages existing ASeq infrastructure and the implementation is easy to understand.

Environment

None

Attachments

19

Activity

Show:

Alex Miller March 25, 2015 at 10:14 PM

updated version of the -15 patch that gives proper credit to Ghadi for part.

Michael Blume March 24, 2015 at 4:53 AM

Yeah, fair enough.

Alex Miller March 24, 2015 at 2:28 AM

The problem with holding a pointer back to the head is that the intervening cycle can be arbitrarily large. You are then literally holding the head on an arbitrary size seq. A silly example that fails with the patch but works without it (depending on your jvm size):

I played with the cycle reduce too but it has the same effective potential issue of creating and holding an arbitrarily sized array. Actually this one is worse in that it realizes an arbitrarily large array before knowing how many inputs it will need - it could be a take 1. Also, things that formerly worked could blow up with an oome. What if someone relies on passing an infinite sequence to cycle in a fallthrough case? For example, this works with the current version but would blow up with the array.

Neither of these seem worth the risk to me.

Michael Blume March 24, 2015 at 1:17 AM

Michael Blume March 24, 2015 at 1:07 AM

Also, for the Cycle reduce implementations, it might make sense to assume the underlying sequence is going to be traversed many times, copy it into an array, and loop over it with an index.

Completed

Details

Assignee

Reporter

Approval

Ok

Patch

Code and Test

Priority

Fix versions

Created November 25, 2014 at 4:13 PM
Updated March 27, 2015 at 8:12 PM
Resolved March 27, 2015 at 8:12 PM