chore: align number hash algorithm with webpack #10643
Conversation
✅ Deploy Preview for rspack canceled.Built without sensitive environment variables
|
CodSpeed Performance ReportMerging #10643 will not alter performanceComparing Summary
|
chore: add long string test cases
|
🤔 Is this a breaking change cc @hardfist |
| let mut hash = FNV_OFFSET_64; | ||
|
|
||
| for code_unit in s.encode_utf16() { | ||
| hash ^= code_unit as u64; |
There was a problem hiding this comment.
This is actually wrong, fnv requires calculations in bytes, not chars.
There was a problem hiding this comment.
I don’t have a strong preference between using the original fnv or the webpack variant. I just assumed the issue was meant to follow webpack’s changes and stay consistent with it — which would also provide a useful reference point for validation.
If your team concludes that the original version is better, just let me know — I can update the code along with the snapshots and test cases.
There was a problem hiding this comment.
I suggestion using original fnv, which avoids the overhead of encode utf16
| } | ||
|
|
||
| pub fn get_number_hash(s: &str, range: usize) -> usize { | ||
| if range < FNV_64_THRESHOLD { |
There was a problem hiding this comment.
I think webpack's choice here doesn't make sense, the performance advantage between 32bit fnv and 64bit fnv doesn't correspond to input length, but cpu arch.
I suggestion just using 64bit fnv1a.
Summary
close #10614
Checklist