Clojure

transient set "keys" and "values" wind up with different metadata

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.6
  • Fix Version/s: None
  • Component/s: None
  • Patch:
    Code and Test

Description

(let [s (-> #{} 
          transient 
          (conj! (clojure.core/with-meta [-7] {:mynum 0}))
          (conj! (clojure.core/with-meta [-7] {:mynum -1})) 
          persistent!)]
  [(meta (s [-7])) (meta (first s))])
=> [{:mynum -1} {:mynum 0}]

basically it looks like the "key" (the value we get by seqing on the set) retains the metadata from the first conj! but the "value" (what we get by calling invoke with the "key") carries the metadata from the second conj!. This does not match the behavior if we don't use transients:

(let [s (-> #{} 
          (conj (clojure.core/with-meta [-7] {:mynum 0}))
          (conj (clojure.core/with-meta [-7] {:mynum -1})))]
  [(meta (s [-7])) (meta (first s))])
=> [{:mynum 0} {:mynum 0}]

(found playing with zach tellman's collection-check)

  1. 0001-CLJ-1615-ensure-transient-set-keys-and-values-have-c.patch
    12/Dec/14 5:43 PM
    2 kB
    Michael Blume
  2. 0001-demonstrate-CLJ-1615.patch
    12/Dec/14 5:07 PM
    1 kB
    Michael Blume
  3. CLJ-1615-entryAt.patch
    13/Dec/14 2:40 PM
    5 kB
    Michael Blume
  4. clj-1615-v3.patch
    06/Oct/17 4:20 PM
    6 kB
    Michael Blume
  5. clj-1615-v4.patch
    06/Oct/17 4:30 PM
    5 kB
    Michael Blume

Activity

Hide
Michael Blume added a comment -

Attached patch demonstrating problem (not a fix)

Show
Michael Blume added a comment - Attached patch demonstrating problem (not a fix)
Hide
Michael Blume added a comment -

More investigation:

The difference between "keys" and "vals" arises from the fact that clojure sets use maps under the covers.

The difference between persistent and transient seems to be because PersistentHashSet.cons short-circuits on contains (https://github.com/clojure/clojure/blob/clojure-1.6.0/src/jvm/clojure/lang/PersistentHashSet.java#L97) and ATransientSet.conj does not (https://github.com/clojure/clojure/blob/clojure-1.6.0/src/jvm/clojure/lang/ATransientSet.java#L27)

Adding a contains check to ATransientSet.conj makes the behavior consistent and passes the attached test, but I imagine this could cause a performance hit. Thoughts?

Show
Michael Blume added a comment - More investigation: The difference between "keys" and "vals" arises from the fact that clojure sets use maps under the covers. The difference between persistent and transient seems to be because PersistentHashSet.cons short-circuits on contains (https://github.com/clojure/clojure/blob/clojure-1.6.0/src/jvm/clojure/lang/PersistentHashSet.java#L97) and ATransientSet.conj does not (https://github.com/clojure/clojure/blob/clojure-1.6.0/src/jvm/clojure/lang/ATransientSet.java#L27) Adding a contains check to ATransientSet.conj makes the behavior consistent and passes the attached test, but I imagine this could cause a performance hit. Thoughts?
Hide
Michael Blume added a comment -

Attached proposed fix – note that this may cause a performance hit for transient sets.

Show
Michael Blume added a comment - Attached proposed fix – note that this may cause a performance hit for transient sets.
Hide
Michael Blume added a comment -

Attaching an alternative fix – instead of doing a contains check on every transient conj, back set.get with entryAt. More invasive but possibly faster.

Show
Michael Blume added a comment - Attaching an alternative fix – instead of doing a contains check on every transient conj, back set.get with entryAt. More invasive but possibly faster.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated: