Skip to content
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

[Compatibility] Added LCS command #843

Merged
merged 18 commits into from
Jan 7, 2025

Conversation

Vijay-Nirmal
Copy link
Contributor

Adding the LCS commands to garnet

  • Add LCS commands
  • Add Integration Test cases, ACL Test and Slot Verification Test
  • Add documentation

@TalZaccai TalZaccai self-requested a review December 3, 2024 19:26
@badrishc
Copy link
Contributor

badrishc commented Dec 5, 2024

Nicely done!!

Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great contribution! This is a gnarly one to implement 👍
A few initial comments... Let me know if you need any further guidance.

libs/server/API/IGarnetApi.cs Outdated Show resolved Hide resolved
libs/server/Resp/ArrayCommands.cs Outdated Show resolved Hide resolved
libs/server/Resp/ArrayCommands.cs Outdated Show resolved Hide resolved
libs/server/Storage/Session/MainStore/MainStoreOps.cs Outdated Show resolved Hide resolved
libs/server/Storage/Session/MainStore/MainStoreOps.cs Outdated Show resolved Hide resolved
libs/server/Storage/Session/MainStore/MainStoreOps.cs Outdated Show resolved Hide resolved
libs/server/Storage/Session/MainStore/MainStoreOps.cs Outdated Show resolved Hide resolved
@microsoft microsoft deleted a comment from TalZaccai Dec 8, 2024
@Vijay-Nirmal
Copy link
Contributor Author

@TalZaccai Fixed review command with an open question

@badrishc
Copy link
Contributor

badrishc commented Dec 8, 2024

Might be a good idea to perf test these commands, varying #threads:

time memtier_benchmark -s <host> -p<port> -Predis --hide-histogram --command="<insert command here>" -c 1 -t 1

another example:

memtier_benchmark -s 10.3.0.54 -p 12002 -c 50 -t 14 \ --command="hset __key__ f1 v1 f2 v2 f3 v3 f4 v4 f5 v5 f6 v6 f7 v7" \ --hide-histogram --test-time 180 --run-count 3

@Vijay-Nirmal
Copy link
Contributor Author

I don't think I have proper setup to run memtier_benchmark, certainly I don't have proper setup to run benchmark to compare again Redis/Valkey. I am running Garnet in windows and running memtier_benchmark in WSL with mirrored networking. I have lot of run to run variants. If you see the below 2 benchmark (one is LCS and another is simple GET) then you can see looks like GET is slower than LCS, also the performance of GET is not consistent. If I rerun the test then I am getting different results

Data Load

MSET key1 ohmytext key2 mynewtext

LCS

nirmal@Nirmal-Omen:~$ memtier_benchmark -c 50 -t 14 --command="LCS key1 key2" --hide-histogram --test-time 120 --run-count 3
Writing results to stdout
[RUN #1] Preparing benchmark client...
[RUN #1] Launching threads now...
[RUN #1 100%, 120 secs]  0 threads:    14981678 ops,  114772 (avg:  124825) ops/sec, 4.93MB/sec (avg: 5.36MB/sec),  6.09 (avg:  5.60) msec latency

[RUN #2] Preparing benchmark client...
[RUN #2] Launching threads now...
[RUN #2 100%, 120 secs]  0 threads:    14388754 ops,  121305 (avg:  119886) ops/sec, 5.21MB/sec (avg: 5.14MB/sec),  5.76 (avg:  5.83) msec latency

[RUN #3] Preparing benchmark client...
[RUN #3] Launching threads now...
[RUN #3 100%, 120 secs]  0 threads:    15949146 ops,  140447 (avg:  132892) ops/sec, 6.03MB/sec (avg: 5.70MB/sec),  4.98 (avg:  5.26) msec latency

14        Threads
50        Connections per thread
120       Seconds


BEST RUN RESULTS
==================================================================================================
Type         Ops/sec    Avg. Latency     p50 Latency     p99 Latency   p99.9 Latency       KB/sec
--------------------------------------------------------------------------------------------------
Lcss       132892.98         5.26037         5.05500        11.51900        23.93500      5840.02
Totals     132892.98         5.26037         5.05500        11.51900        23.93500     11680.05


WORST RUN RESULTS
==================================================================================================
Type         Ops/sec    Avg. Latency     p50 Latency     p99 Latency   p99.9 Latency       KB/sec
--------------------------------------------------------------------------------------------------
Lcss       119889.36         5.83048         5.56700        12.92700        29.05500      5268.58
Totals     119889.36         5.83048         5.56700        12.92700        29.05500     10537.15


AGGREGATED AVERAGE RESULTS (3 runs)
==================================================================================================
Type         Ops/sec    Avg. Latency     p50 Latency     p99 Latency   p99.9 Latency       KB/sec
--------------------------------------------------------------------------------------------------
Lcss       126120.02         5.55366         5.31100        12.09500        26.11100      5542.38
Totals     126120.02         5.55366         5.31100        12.09500        26.11100     11084.77

GET

nirmal@Nirmal-Omen:~$ memtier_benchmark -c 50 -t 14 --command="GET key1" --hide-histogram --test-time 120 --run-count 3
Writing results to stdout
[RUN #1] Preparing benchmark client...
[RUN #1] Launching threads now...
[RUN #1 100%, 120 secs]  0 threads:    16169842 ops,  113057 (avg:  134725) ops/sec, 3.99MB/sec (avg: 4.75MB/sec),  6.18 (avg:  5.19) msec latency

[RUN #2] Preparing benchmark client...
[RUN #2] Launching threads now...
[RUN #2 100%, 120 secs]  0 threads:    10675355 ops,   76615 (avg:   88939) ops/sec, 2.70MB/sec (avg: 3.14MB/sec),  9.11 (avg:  7.86) msec latency

[RUN #3] Preparing benchmark client...
[RUN #3] Launching threads now...
[RUN #3 100%, 120 secs]  0 threads:    10032443 ops,   90045 (avg:   83583) ops/sec, 3.18MB/sec (avg: 2.95MB/sec),  7.77 (avg:  8.36) msec latency

14        Threads
50        Connections per thread
120       Seconds


BEST RUN RESULTS
==================================================================================================
Type         Ops/sec    Avg. Latency     p50 Latency     p99 Latency   p99.9 Latency       KB/sec
--------------------------------------------------------------------------------------------------
Gets       134726.19         5.18910         4.95900        10.30300        19.19900      4868.04
Totals     134726.19         5.18910         4.95900        10.30300        19.19900      9736.07


WORST RUN RESULTS
==================================================================================================
Type         Ops/sec    Avg. Latency     p50 Latency     p99 Latency   p99.9 Latency       KB/sec
--------------------------------------------------------------------------------------------------
Gets        83565.60         8.36044         8.15900        22.52700        46.07900      3019.46
Totals      83565.60         8.36044         8.15900        22.52700        46.07900      6038.92


AGGREGATED AVERAGE RESULTS (3 runs)
==================================================================================================
Type         Ops/sec    Avg. Latency     p50 Latency     p99 Latency   p99.9 Latency       KB/sec
--------------------------------------------------------------------------------------------------
Gets       102476.23         6.82447         6.49500        15.99900        39.67900      3702.75
Totals     102476.23         6.82447         6.49500        15.99900        39.67900      7405.51

@badrishc @TalZaccai Provided the inconsistency with my setup of memtier_benchmark, I am not going to use that for my performance testing of LCS command. I will use BDN.Benchmark of Garnet to perf test the LCS command and check I can do something to optimize.

Feel free to guide me in the right direction if I am doing something wrong with memtier_benchmark. I haven't used memtier_benchmark before, so most definitely I am doing something wrong.

@TalZaccai TalZaccai merged commit 56394d8 into microsoft:main Jan 7, 2025
15 checks passed
kevin-montrose pushed a commit that referenced this pull request Jan 15, 2025
* Added LCS command

* Format fix

* Reverted CommandDocsUpdater.cs

* Fix cluster test

* Fixed wrong change

* Moved to constant

* Review command fixes

* Fixed review comment

* Fixed test issue

---------

Co-authored-by: Vasileios Zois <[email protected]>
Co-authored-by: Tal Zaccai <[email protected]>
darrenge added a commit that referenced this pull request Jan 16, 2025
* Lua allocations go through .NET; really crummy allocator on the POH just to prove it's possible; squashing to clean up _a lot_ of experimentation commits

* punch a LuaOptions into settings

* wire up the Lua options

* prep for other allocators

* Implement more allocators and expand testing

* Knock out a number of todos, consider allocator in benchmarks, additional validation

* add a test for OOMs

* formatting

* convert ScriptOperations to explore different allocators

* cleanup Lua error messgages; this revealed a bug in buffer management for LuaScripts, fixes that

* Make managed allocator less naive, and benchmark on par.
It will probably be unusual to use this allocator, but it shouldn't be _bad_ either.

* formatting

* address nit

* address feedback; only copy relevant bits, not whole buffer

* set lua options in OperationsBase, fixing benchmarks

* BDN Updates:
1) Added a check for NA in results which is an indication that the BDN test failed at run time
2) Added 'Lua.LuaScriptCacheOperations','Lua.LuaRunnerOperations' to BDN Github Action
3) Updated Expected values for the new Lua BDN tests

* Updated some of the LuaScriptCacheOperations expected values

* [Compatibility] Added LCS command (#843)

* Added LCS command

* Format fix

* Reverted CommandDocsUpdater.cs

* Fix cluster test

* Fixed wrong change

* Moved to constant

* Review command fixes

* Fixed review comment

* Fixed test issue

---------

Co-authored-by: Vasileios Zois <[email protected]>
Co-authored-by: Tal Zaccai <[email protected]>

* Configure min and max IO completion threads (#904)

* Configure min and max IO completion threads separately from min and max threads (in the ThreadPool). This is needed as some scenarios may limit number of thread pool threads but require a larger number of IO completion threads.

* nit

* address nit

* address feedback; only copy relevant bits, not whole buffer

* set lua options in OperationsBase, fixing benchmarks

* Yak shave a bunch of cleanup, looking to reduce amount of work done generally in Lua to get some perf back.

* formatting

* fix LuaRunnerOperations ; session wouldn't always be initialized before, which would cause SendAndReset() to fail thinking the message was too large

* Updated Expected values for Allocated in the BDN perf tests to match current Lua changes.

* Missed one expected value. Fixed that so should be ok.

* Seen some instances where allocated bytes are 1024 on one run and then run it a second time and see 1312 without any code changes. Just a small variance between runs so decided to just make sure doesn't go above 1312 for any that were set to 1024.  The charts and other things will show the small nuances if needed.

* Bumped expected values up a bit to handle variance

---------

Co-authored-by: darrenge <[email protected]>
Co-authored-by: Vijay Nirmal <[email protected]>
Co-authored-by: Vasileios Zois <[email protected]>
Co-authored-by: Tal Zaccai <[email protected]>
Co-authored-by: Badrish Chandramouli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants