Affects Version/s: None
Fix Version/s: Release 1.7
Patch:Code and Test
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
- 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
Performance: I tested perf using criterium to benchmark as follows:
|(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|
- 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
- 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)
- the changes in CollReduce are unlikely to stand but are currently essential