Clojure

Records do not cache hash like normal maps

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Patch:
    Code
  • Approval:
    Vetted

Description

Records do not cache their hash codes like normal Clojure maps, which affects their performance. This problem has been fixed in CLJS, but still affects JVM CLJ.

Approach: Cache hash values in record definitions, similar to maps.

Patch: 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch

Also see: http://dev.clojure.org/jira/browse/CLJS-281

Activity

Nicola Mometto made changes -
Field Original Value New Value
Attachment 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch [ 12041 ]
Brandon Bloom made changes -
Labels patch patch,
Andy Fingerhut made changes -
Patch Code [ 10001 ]
Alex Miller made changes -
Labels patch patch,
Alex Miller made changes -
Approval Triaged [ 10120 ]
Alex Miller made changes -
Priority Minor [ 4 ] Critical [ 2 ]
Alex Miller made changes -
Labels defrecord performance
Alex Miller made changes -
Description This came up with dnolen and ambrose in irc. Apparently it was also an issue for Mark Engleberg with Instaparse. This has been fixed in CLJS, but still affects JVM CLJ.

See http://dev.clojure.org/jira/browse/CLJS-281
Records do not cache their hash codes like normal Clojure maps, which affects their performance. This problem has been fixed in CLJS, but still affects JVM CLJ.

*Approach:* Cache hash values in record definitions, similar to maps.

*Patch:* 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch

*Also see:* http://dev.clojure.org/jira/browse/CLJS-281
Issue Type Defect [ 1 ] Enhancement [ 4 ]
Alex Miller made changes -
Priority Critical [ 2 ] Major [ 3 ]
Hide
Nicola Mometto added a comment -

I want to point out that my patch breaks ABI compatibility.
A possible approach to avoid this would be to have 3 constructors instead of 2, I can write the patch to support this if desired.

Show
Nicola Mometto added a comment - I want to point out that my patch breaks ABI compatibility. A possible approach to avoid this would be to have 3 constructors instead of 2, I can write the patch to support this if desired.
Rich Hickey made changes -
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Rich Hickey made changes -
Fix Version/s Release 1.7 [ 10250 ]
Hide
Stuart Halloway added a comment -

The patch 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch is broken in at least two ways:

  • The fields __hash and __hasheq are adopted by new records created by .assoc and .without, which will cause those records to have incorrect (and likely colliding) hash values
  • The addition of the new fields breaks the promise of defrecord, which includes an N+2 constructor taking meta and extmap. With the patch, defrecords get an N+4 constructor letting callers pick hash codes.

I found these problems via the following reasoning:

  • Code has been touched near __extmap
  • Grep for all uses of __extmap and see what breaks
Show
Stuart Halloway added a comment - The patch 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch is broken in at least two ways:
  • The fields __hash and __hasheq are adopted by new records created by .assoc and .without, which will cause those records to have incorrect (and likely colliding) hash values
  • The addition of the new fields breaks the promise of defrecord, which includes an N+2 constructor taking meta and extmap. With the patch, defrecords get an N+4 constructor letting callers pick hash codes.
I found these problems via the following reasoning:
  • Code has been touched near __extmap
  • Grep for all uses of __extmap and see what breaks
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Hide
Nicola Mometto added a comment -

Patch 0001-cache-hasheq-and-hashCode-for-records.patch fixes both those issues, reintroducing the N+2 arity constructor

Show
Nicola Mometto added a comment - Patch 0001-cache-hasheq-and-hashCode-for-records.patch fixes both those issues, reintroducing the N+2 arity constructor
Nicola Mometto made changes -
Hide
Alex Miller added a comment -

Questions addressed, back to Vetted.

Show
Alex Miller added a comment - Questions addressed, back to Vetted.
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]

People

Vote (8)
Watch (7)

Dates

  • Created:
    Updated: