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.8
  • 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

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.
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
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
Hide
Alex Miller added a comment -

Questions addressed, back to Vetted.

Show
Alex Miller added a comment - Questions addressed, back to Vetted.
Hide
Andy Fingerhut added a comment -

All patches dated Jun 7 2014 and earlier no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. They did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch.

Show
Andy Fingerhut added a comment - All patches dated Jun 7 2014 and earlier no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. They did apply cleanly before that day. I have not checked how easy or difficult it might be to update this patch.
Hide
Alex Miller added a comment -

Would be great to get this one updated as it's otherwise ready to screen.

Show
Alex Miller added a comment - Would be great to get this one updated as it's otherwise ready to screen.
Hide
Nicola Mometto added a comment -

Updated patch to apply to lastest master

Show
Nicola Mometto added a comment - Updated patch to apply to lastest master

People

Vote (9)
Watch (8)

Dates

  • Created:
    Updated: