mikemccand commented on code in PR #12716:
URL: https://github.com/apache/lucene/pull/12716#discussion_r1370148505


##########
lucene/CHANGES.txt:
##########
@@ -190,6 +190,8 @@ Improvements
 * GITHUB#12705, GITHUB#12705: Improve handling of NullPointerException and 
IllegalStateException
   in MMapDirectory's IndexInputs.  (Uwe Schindler, Michael Sokolov)
 
+* GITHUB#12716: Improve hash mixing in FST's double-barrel LRU hash. (Shubham 
Chaudhary)

Review Comment:
   Maybe also add something more end-user friendly like `, making FST 
compilation faster and more RAM-efficient` or so?



##########
lucene/core/src/java/org/apache/lucene/util/fst/NodeHash.java:
##########
@@ -182,8 +183,9 @@ private long hash(FSTCompiler.UnCompiledNode<T> node) {
         h += 17;
       }
     }
-
-    return h;
+    // Here we multiply the hash with the gold constant BitMixer#PHI_C64
+    // This makes a real difference in the evenness of the value distribution
+    return h * BitMixer.PHI_C64;

Review Comment:
   Could you also add a link that @dweiss provided in [this 
comment](https://github.com/apache/lucene/issues/12704#issuecomment-1774156970)?
  I always freak out a bit when I see magical constants and I don't understand 
their magic.  [That 
link](https://softwareengineering.stackexchange.com/questions/402542/where-do-magic-hashing-constants-like-0x9e3779b9-and-0x9e3779b1-come-from)
 goes into a nice little complex history lesson...).  Or perhaps put the link 
on the `BitMixer.PHI_C64` as a comment?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org

Reply via email to