Completed
Details
Assignee
UnassignedUnassignedReporter
Stuart HallowayStuart HallowayApproval
OkPatch
Code and TestPriority
MajorFix versions
Details
Details
Assignee
Unassigned
UnassignedReporter
Stuart Halloway
Stuart HallowayApproval
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
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