This actually applies to all three map types. (In fact, in (-conj {} "foo"), the map is an array map.) In Clojure, conj on a map works with a number of argument types:

1. map entries;

2. two-element vectors;

3. seqables of map entries.

The final case is, perhaps surprisingly, the oldest one. Merging maps falls under it, since for map arguments it boils down to merge minus special treatment of nil (merge uses conj to merge pairs of maps); but arbitrary seqables of map entries are supported. (NB. these must be actual map entries, not two-element vectors!) This allows one, for example, to filter a map and conj the result of that into another map.

So, we want to support the legitimate use cases while maybe complaining about code that wouldn't work in Clojure if it's not too much of a problem performance-wise. An example of a call that we'd probably like to throw: {{(conj {} (list (list [:foo 1])))}}.

The attached patch makes the -conj implementations in all the map types use an explicit loop in the non-vector branch and adds some test for the resulting behaviour.

Michał Marczyk
added a comment - 22/Apr/14 6:13 AM - edited This actually applies to all three map types. (In fact, in (-conj {} "foo"), the map is an array map.) In Clojure, conj on a map works with a number of argument types:
1. map entries;
2. two-element vectors;
3. seqables of map entries.
The final case is, perhaps surprisingly, the oldest one. Merging maps falls under it, since for map arguments it boils down to merge minus special treatment of nil (merge uses conj to merge pairs of maps); but arbitrary seqables of map entries are supported. (NB. these must be actual map entries, not two-element vectors!) This allows one, for example, to filter a map and conj the result of that into another map.
So, we want to support the legitimate use cases while maybe complaining about code that wouldn't work in Clojure if it's not too much of a problem performance-wise. An example of a call that we'd probably like to throw: {{(conj {} (list (list [:foo 1])))}}.
The attached patch makes the -conj implementations in all the map types use an explicit loop in the non-vector branch and adds some test for the resulting behaviour.

I guess implementing INext is not part of the contract for ISeqable.-seq, which means NodeSeq doesn't have to implement it, right?
In that case, the right fix is to use next instead of -next inside of Map.-conj, when dealing with a (possibly user defined) seq of MapEntries.

Herwig Hochleitner
added a comment - 06/May/14 6:46 AM I guess implementing INext is not part of the contract for ISeqable.-seq, which means NodeSeq doesn't have to implement it, right?
In that case, the right fix is to use next instead of -next inside of Map.-conj, when dealing with a (possibly user defined) seq of MapEntries.
Attached patch 0002 uses next instead of -next and adds tests for map-entry seqs not implementing INext

Another approach would be to use reduce, hopefully benefiting from IReduce speed boosts. Of course we'd need to use a custom reduction function wrapping -conj with a vector? check. The attached patch implements this.

Michał Marczyk
added a comment - 06/May/14 3:04 PM Good catch, thanks!
Another approach would be to use reduce, hopefully benefiting from IReduce speed boosts. Of course we'd need to use a custom reduction function wrapping -conj with a vector? check. The attached patch implements this.

Actually, scratch the part about IReduce speed boosts – sorry for the confusion!

Having run better benchmarks with the two patches on a recent build of V8 and I have to say that there doesn't seem to be much of a difference and actually the next-based approach comes out ahead sometimes. In Clojure, a hand-rolled loop-based "map-seq-conj" loses to a hand-rolled reduce-based impl consistently, as far as I can tell, although only by ~3-5%. I've been conj-ing seqs over vectors of vectors, which should be friendly to reduce.

Michał Marczyk
added a comment - 06/May/14 3:49 PM Actually, scratch the part about IReduce speed boosts – sorry for the confusion!
Having run better benchmarks with the two patches on a recent build of V8 and I have to say that there doesn't seem to be much of a difference and actually the next-based approach comes out ahead sometimes. In Clojure, a hand-rolled loop-based "map-seq-conj" loses to a hand-rolled reduce-based impl consistently, as far as I can tell, although only by ~3-5%. I've been conj-ing seqs over vectors of vectors, which should be friendly to reduce.