vals and keys return values should implement IReduceInit or Iterable

Description

Background: clojure.core/keys calls RT.keys(Object) calls APersistentMap.KeySeq.create(ISeq). RT.keys() creates a sequence (of Map.Entry objects) and KeySeq just wraps it, calling .keyValue(). There is an equivalent vals -> RT.vals() -> ValSeq path. Both of these seq impls extend ASeq and provide Iterable implementations via SeqIterator (iterator wrapped over the seq).

Approach: The important thing here is to avoid creating the sequence and instead directly iterate/reduce over the map. Noting that CLJ-1499 provides support for making PHM directly Iterable and that KeySeq/ValSeq already implement Iterable, I chose to focus on making the instances returned from keys and vals support Iterable directly on the underlying map instead of via the seq.

RT.keys()/vals() created the seq and passed it to KeySeq/ValSeq which made it too late to directly cover the original map iterator. There are a few places that rely on passing a seq of Map.Entry to keys/vals (not just a map instance), so I check for IPersistentMap and in that case pass it directly to a new KeySeq factory method that remembers both the Iterable and the ISeq. There is also support in here for using a direct key/value iterator via IMapIterable as introduced in CLJ-1499.

Questions/notes:

  • Could potentially check for Map or Iterable instead of IPersistentMap in RT.keys()/vals(). Not sure how common it is to pass normal Java maps to keys/vals.

  • The direct Iterable support vanishes once you move off the head of the keys or vals seq. So (rest (keys map)) does not have Iterable support. This is not really possible unless you hold an Iterator and advance it along with the seq, but that seemed to introduce all sorts of possibilities for badness. Since maps are unordered, it seems weird to rely on any ordering or processing only parts of any map, so I suspect doing this would be quite rare.

  • This patch depends on CLJ-1499 for IMapIterable.

Performance: I tested perf using criterium to benchmark as follows:

(use 'criterium.core) (def m (zipmap (range 1000) (range 1000))) (bench (reduce + (keys m))) (bench (reduce + (vals m)))

(bench expr)

1.6.0

1.7.0-alpha5

alpha5 + clj-1499 + patch

(reduce + (keys m))

69 µs

73 µs

44 µs

(reduce + (vals m))

75 µs

77 µs

50 µs

Tests:

  • I added some basic tests for subseq and rsubseq as those both rely on the somewhat special behavior of keys accepting a seqable of Map.Entry objects (not just a map itself). There were no other tests for subseq or rsubseq already present.

  • Some coverage from CLJ-1499 tests d760db

  • Show that metadata works on keys/vals

  • Show that iterator works correctly after you step off the head of the seq, invalidating the iterable

  • Show that the fallthrough branch of iterator works (from PersistentTreeMap)

Patch: clj-1602-6.diff

Screened by:

  • the changes in CollReduce are unlikely to stand but are currently essential

Environment

None

Attachments

6
  • 23 Mar 2015, 07:51 PM
  • 22 Mar 2015, 02:16 PM
  • 14 Jan 2015, 11:40 PM
  • 04 Dec 2014, 07:36 PM
  • 04 Dec 2014, 05:24 AM
  • 03 Dec 2014, 11:19 PM

Activity

Show:

Alex Miller March 23, 2015 at 7:51 PM

Updated to -6 patch that adds the requested tests.

Stuart Halloway March 22, 2015 at 2:37 PM

requested some tests in the description

Stuart Halloway March 22, 2015 at 2:16 PM

clj-1602-5.diff merely updates -4 to apply on current master

Michael Blume January 15, 2015 at 4:10 PM

Ah, cool =)

Alex Miller January 15, 2015 at 1:21 PM

Actually I did see it in the generative tests for this ticket so I moved those tests into the CLJ-1499 patch.

Completed

Details

Assignee

Reporter

Approval

Ok

Patch

Code and Test

Priority

Fix versions

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