-
Notifications
You must be signed in to change notification settings - Fork 637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved hashing algorithm in luaS_newlstr #1168
Improved hashing algorithm in luaS_newlstr #1168
Conversation
Signed-off-by: Shai Zarka <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1168 +/- ##
============================================
- Coverage 70.69% 70.63% -0.06%
============================================
Files 114 114
Lines 61693 61740 +47
============================================
+ Hits 43611 43613 +2
- Misses 18082 18127 +45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
Are Lua tables using this hash function too? A Lua script that just creates large lua tables and looks up elements in it would benefit much too, right?
Lua is a vendored dependency. Having patches to this code makes it harder to update the dependency. I'm not sure we will ever update this though, because later Lua versions are not 100% compatible with 5.1, but in general.
In deps/README.md, it's described how to update each dep. We should mention there that we have changed Lua's hash function.
Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: zarkash-aws <[email protected]>
Signed-off-by: Shai Zarka <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Minor improvements. Signed-off-by: Madelyn Olson <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]> Signed-off-by: Madelyn Olson <[email protected]>
**Overview** This PR introduces the use of [MurmurHash3](https://en.wikipedia.org/wiki/MurmurHash) as the hashing function for Lua's luaS_newlstr function, replacing the previous simple hash function. The change aims to improve performance, particularly for large strings. **Changes** Implemented MurmurHash3 algorithm in lstring.c Updated luaS_newlstr to use MurmurHash3 for string hashing **Performance Testing:** Test Setup: 1. Ran a valkey server 2. Loaded 1000 keys with large values (100KB each) to the server using a Lua script ``` local numKeys = 1000 for i = 1, numKeys do local key = "large_key_" .. i local largeValue = string.rep("x", 1024*100) redis.call("SET", key, largeValue) end ``` 3. Used a Lua script to randomly select and retrieve keys ``` local randomKey = redis.call("RANDOMKEY") local result = redis.call("GET", randomKey) ``` 4. Benchmarked using valkey-benchmark: `./valkey-benchmark -n 100000 evalsha c157a37967e69569339a39a953c046fc2ecb4258 0` Results: A | Unstable | This PR | Change -- | -- | -- | -- Throughput | 6,835.74 requests per second | 17,061.94 requests per second | **+150% increase** Avg Latency | 7.218 ms | 2.838 ms | **-61% decrease** Min Latency | 3.144 ms | 1.320 ms | **-58% decrease** P50 Latency | 8.463 ms | 3.167 ms | **-63% decrease** P95 Latency | 8.863 ms | 3.527 ms | **-60% decrease** P99 Latency | 9.063 ms | 3.663 ms | **-60% decrease** Max Latency | 63.871 ms | 55.327 ms | **-13% decrease** Summary: * Throughput: Improved by 150%. * Latency: Significant reductions in average, minimum, and percentile latencies (P50, P95, P99), leading to much faster response times. * Max Latency: Slightly decreased by 13%, indicating fewer outlier delays after the fix. --------- Signed-off-by: Shai Zarka <[email protected]> Signed-off-by: zarkash-aws <[email protected]> Signed-off-by: Madelyn Olson <[email protected]> Co-authored-by: Madelyn Olson <[email protected]> Co-authored-by: Viktor Söderqvist <[email protected]>
Overview
This PR introduces the use of MurmurHash3 as the hashing function for Lua's luaS_newlstr function, replacing the previous simple hash function. The change aims to improve performance, particularly for large strings.
Changes
Implemented MurmurHash3 algorithm in lstring.c
Updated luaS_newlstr to use MurmurHash3 for string hashing
Performance Testing:
Test Setup:
./valkey-benchmark -n 100000 evalsha c157a37967e69569339a39a953c046fc2ecb4258 0
Results:
Summary: