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.

Sign up for GitHub

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

Jump to bottom

[Compatibility] Add hash expiration - HEXPIRE, HPEXPIRE, HEXPIREAT, HPEXPIREAT, HTTL, HPTTL, HEXPIRETIME, HPEXPIRETIME, HPERSIST and HCOLLECT #864

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

[Compatibility] Add hash expiration - HEXPIRE, HPEXPIRE, HEXPIREAT, HPEXPIREAT, HTTL, HPTTL, HEXPIRETIME, HPEXPIRETIME, HPERSIST and HCOLLECT #864

wants to merge 36 commits into from

Conversation

Vijay-Nirmal
Copy link
Contributor

@Vijay-Nirmal Vijay-Nirmal commented Dec 9, 2024 •
edited
Loading

Adding support for field-specific expiration in hash set. This PR adds HEXPIRE, HPEXPIRE, HEXPIREAT, HPEXPIREAT, HTTL, HPTTL, HEXPIRETIME, HPEXPIRETIME, HPERSIST and HCOLLECT commands to garnet to support this feature.

  • Implementation of HEXPIRE, HPEXPIRE, HEXPIREAT, HPEXPIREAT, HTTL, HPTTL, HEXPIRETIME, HPEXPIRETIME and HPERSIST commands
  • Fix all the TODO comments
  • Implementation of HCOLLECT to delete expired fields on demand
  • Add Integration Test cases to get all the new commands
  • Add Integration to test all the existing Hash set commands with expiring fields
  • ACL Test
  • Slot Verification Test
  • Add documentation
  • Add a background task to call HCOLLECT to delete expired items

Fixes: #857

@Vijay-Nirmal
Copy link
Contributor Author

@badrishc Can you quick look at this draft PR and let me know if I am going on the right path? Also, you will see a lot of TODO comments, I would like to get your opinion and help on some of those. Add your comments on those TODO items if possible. Thanks

@badrishc
Copy link
Contributor

badrishc commented Dec 9, 2024

For maintaining the performance bar, we may need to:

(1) Have a separate small PR to main, that adds BDN for more of the hash APIs for perf test (i.e., the ones that this PR might potentially regress). We only have one right now here:

https://github.com/microsoft/garnet/blob/main/benchmark/BDN.benchmark/Operations/ObjectOperations.cs#L26

(2) Use those BDNs to compare main versus this PR, to ensure that there is no regression

badrishc
badrishc requested changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@badrishc badrishc left a comment

Choose a reason for hiding this comment

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

This is a great start! See my comments, thank you. Let me know if anything is still unclear.

libs/server/Objects/Hash/HashObject.cs Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObject.cs Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObject.cs Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObject.cs Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObject.cs Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObject.cs Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObject.cs Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObject.cs Outdated Show resolved Hide resolved
test/Garnet.test/RespHashTests.cs Outdated Show resolved Hide resolved
@Vijay-Nirmal Vijay-Nirmal marked this pull request as ready for review December 12, 2024 18:47
@Vijay-Nirmal
Copy link
Contributor Author

Vijay-Nirmal commented Dec 12, 2024 •
edited
Loading

I think it's ready for review. Let the review comments flood it 😄

@badrishc I am working on creating a separate PR for adding more hash APIs in BDN

@vazois vazois self-requested a review December 12, 2024 21:17
@Vijay-Nirmal
Copy link
Contributor Author

Vijay-Nirmal commented Dec 13, 2024 •
edited
Loading

For maintaining the performance bar, we may need to:

(1) Have a separate small PR to main, that adds BDN for more of the hash APIs for perf test (i.e., the ones that this PR might potentially regress). We only have one right now here:

https://github.com/microsoft/garnet/blob/main/benchmark/BDN.benchmark/Operations/ObjectOperations.cs#L26

(2) Use those BDNs to compare main versus this PR, to ensure that there is no regression

@badrishc

  1. Raised the PR will all Hash commands Added BDN for all hash commands #879
  2. Below are the results
dotnet run -c Release -f net8.0 --filter BDN.benchmark.Operations.ObjectOperations.H*

main branch results as of commit aa004d372a23731e3e5f0fa65d701c4a4d62b381

Method Params Mean Error StdDev Allocated
HSetDel ACL 71.403 us 0.7838 us 0.7332 us 16000 B
HExists ACL 6.898 us 0.0644 us 0.0571 us -
HGet ACL 5.938 us 0.0806 us 0.0754 us -
HGetAll ACL 5.215 us 0.0906 us 0.0847 us -
HIncrby ACL 7.586 us 0.1066 us 0.0997 us -
HIncrbyFloat ACL 7.942 us 0.0769 us 0.0720 us -
HKeys ACL 6.514 us 0.1299 us 0.1596 us -
HLen ACL 5.756 us 0.0913 us 0.0854 us -
HMGet ACL 6.544 us 0.1303 us 0.1694 us -
HMSet ACL 7.998 us 0.1584 us 0.1404 us -
HRandField ACL 6.787 us 0.0415 us 0.0388 us -
HScan ACL 6.289 us 0.0630 us 0.0589 us 776 B
HSetNx ACL 6.851 us 0.0389 us 0.0345 us -
HStrLen ACL 7.494 us 0.0713 us 0.0632 us -
HVals ACL 5.823 us 0.0525 us 0.0491 us -
HSetDel AOF 78.643 us 1.1582 us 1.0834 us 16000 B
HExists AOF 27.838 us 0.2605 us 0.2437 us 6400 B
HGet AOF 28.008 us 0.2615 us 0.2446 us 6400 B
HGetAll AOF 30.968 us 0.3035 us 0.2839 us 3200 B
HIncrby AOF 43.694 us 0.4504 us 0.3993 us 9600 B
HIncrbyFloat AOF 61.934 us 0.8169 us 0.7641 us 14400 B
HKeys AOF 30.483 us 0.4016 us 0.3757 us 3200 B
HLen AOF 24.903 us 0.1990 us 0.1861 us 3200 B
HMGet AOF 32.191 us 0.2342 us 0.2076 us 9600 B
HMSet AOF 43.277 us 0.3919 us 0.3666 us 16000 B
HRandField AOF 35.159 us 0.2426 us 0.2269 us 8800 B
HScan AOF 6.350 us 0.0436 us 0.0408 us 776 B
HSetNx AOF 38.265 us 0.4156 us 0.3684 us 9600 B
HStrLen AOF 28.177 us 0.2426 us 0.2150 us 6400 B
HVals AOF 29.212 us 0.1524 us 0.1426 us 3200 B
HSetDel None 68.038 us 0.6442 us 0.6026 us 16000 B
HExists None 28.753 us 0.2215 us 0.2072 us 6400 B
HGet None 26.804 us 0.1627 us 0.1521 us 6400 B
HGetAll None 31.720 us 0.2792 us 0.2475 us 3200 B
HIncrby None 37.302 us 0.3543 us 0.3314 us 9600 B
HIncrbyFloat None 57.083 us 0.5250 us 0.4910 us 14400 B
HKeys None 29.819 us 0.2232 us 0.1978 us 3200 B
HLen None 24.448 us 0.2692 us 0.2518 us 3200 B
HMGet None 31.395 us 0.3747 us 0.3505 us 9600 B
HMSet None 38.002 us 0.3554 us 0.3324 us 16000 B
HRandField None 35.847 us 0.2908 us 0.2720 us 8800 B
HScan None 6.302 us 0.0562 us 0.0526 us 776 B
HSetNx None 32.562 us 0.3962 us 0.3706 us 9600 B
HStrLen None 27.899 us 0.2199 us 0.1949 us 6400 B
HVals None 28.536 us 0.1803 us 0.1598 us 3200 B

this PR branch results as of commit fa20261ba8f16d18a7826884825a8461e1e56f45

Method Params Mean Error StdDev Allocated
HSetDel ACL 73.189 us 0.9091 us 0.8504 us 16000 B
HExists ACL 6.552 us 0.0744 us 0.0696 us -
HGet ACL 5.830 us 0.0871 us 0.0814 us -
HGetAll ACL 5.610 us 0.0550 us 0.0514 us -
HIncrby ACL 7.885 us 0.0712 us 0.0631 us -
HIncrbyFloat ACL 9.277 us 0.1333 us 0.1247 us -
HKeys ACL 5.332 us 0.0668 us 0.0592 us -
HLen ACL 5.224 us 0.0531 us 0.0497 us -
HMGet ACL 6.251 us 0.0520 us 0.0486 us -
HMSet ACL 6.923 us 0.0620 us 0.0518 us -
HRandField ACL 7.561 us 0.0626 us 0.0586 us -
HScan ACL 6.493 us 0.0446 us 0.0417 us 776 B
HSetNx ACL 6.623 us 0.0369 us 0.0327 us -
HStrLen ACL 8.455 us 0.0510 us 0.0452 us -
HVals ACL 5.215 us 0.0431 us 0.0382 us -
HSetDel AOF 82.861 us 1.1019 us 1.0307 us 16000 B
HExists AOF 28.262 us 0.1794 us 0.1591 us 6400 B
HGet AOF 29.512 us 0.1529 us 0.1430 us 6400 B
HGetAll AOF 36.729 us 0.4533 us 0.4240 us 8800 B
HIncrby AOF 44.028 us 0.4440 us 0.4153 us 9600 B
HIncrbyFloat AOF 63.666 us 0.9256 us 0.8658 us 14400 B
HKeys AOF 33.583 us 0.4228 us 0.3955 us 8800 B
HLen AOF 26.797 us 0.1521 us 0.1348 us 3200 B
HMGet AOF 32.715 us 0.4234 us 0.3960 us 9600 B
HMSet AOF 49.478 us 0.3561 us 0.3331 us 16000 B
HRandField AOF 36.244 us 0.3035 us 0.2839 us 8800 B
HScan AOF 6.312 us 0.0463 us 0.0433 us 776 B
HSetNx AOF 39.744 us 0.5762 us 0.5390 us 9600 B
HStrLen AOF 30.031 us 0.4417 us 0.3916 us 6400 B
HVals AOF 35.003 us 0.4019 us 0.3759 us 8800 B
HSetDel None 68.766 us 0.6765 us 0.6328 us 16000 B
HExists None 28.940 us 0.1759 us 0.1645 us 6400 B
HGet None 28.242 us 0.1756 us 0.1642 us 6400 B
HGetAll None 36.430 us 0.3877 us 0.3626 us 8800 B
HIncrby None 38.859 us 0.5240 us 0.4901 us 9600 B
HIncrbyFloat None 56.103 us 0.6336 us 0.5291 us 14400 B
HKeys None 33.537 us 0.2808 us 0.2626 us 8800 B
HLen None 26.031 us 0.1829 us 0.1710 us 3200 B
HMGet None 31.914 us 0.3328 us 0.3113 us 9600 B
HMSet None 43.008 us 0.5230 us 0.4892 us 16000 B
HRandField None 36.078 us 0.2446 us 0.2288 us 8800 B
HScan None 6.371 us 0.0487 us 0.0456 us 776 B
HSetNx None 33.417 us 0.3972 us 0.3715 us 9600 B
HStrLen None 30.825 us 0.3913 us 0.3661 us 6400 B
HVals None 34.041 us 0.3687 us 0.3449 us 8800 B

Difference PR - Main

Method Mean Error StdDev Allocated
HSetDel 1.786 0.1253 0.1172 0
HExists -0.346 0.01 0.0125 0
HGet -0.108 0.0065 0.006 0
HGetAll 0.395 -0.0356 -0.0333 0
HIncrby 0.299 -0.0354 -0.0366 0
HIncrbyFloat 1.335 0.0564 0.0527 0
HKeys -1.182 -0.0631 -0.1004 0
HLen -0.532 -0.0382 -0.0357 0
HMGet -0.293 -0.0783 -0.1208 0
HMSet -1.075 -0.0964 -0.0886 0
HRandField 0.774 0.0211 0.0198 0
HScan 0.204 -0.0184 -0.0172 0
HSetNx -0.228 -0.002 -0.0018 0
HStrLen 0.961 -0.0203 -0.018 0
HVals -0.608 -0.0094 -0.0109 0
HSetDel 4.218 -0.0563 -0.0527 0
HExists 0.424 -0.0811 -0.0846 0
HGet 1.504 -0.1086 -0.1016 0
HGetAll 5.761 0.1498 0.1401 5600
HIncrby 0.334 -0.0064 0.016 0
HIncrbyFloat 1.732 0.1087 0.1017 0
HKeys 3.1 0.0212 0.0198 5600
HLen 1.894 -0.0469 -0.0513 0
HMGet 0.524 0.1892 0.1884 0
HMSet 6.201 -0.0358 -0.0335 0
HRandField 1.085 0.0609 0.057 0
HScan -0.038 0.0027 0.0025 0
HSetNx 1.479 0.1606 0.1706 0
HStrLen 1.854 0.1991 0.1766 0
HVals 5.791 0.2495 0.2333 5600
HSetDel 0.728 0.0323 0.0302 0
HExists 0.187 -0.0456 -0.0427 0
HGet 1.438 0.0129 0.0121 0
HGetAll 4.71 0.1085 0.1151 5600
HIncrby 1.557 0.1697 0.1587 0
HIncrbyFloat -0.98 0.1086 0.0381 0
HKeys 3.718 0.0576 0.0648 5600
HLen 1.583 -0.0863 -0.0808 0
HMGet 0.519 -0.0419 -0.0392 0
HMSet 5.006 0.1676 0.1568 0
HRandField 0.231 -0.0462 -0.0432 0
HScan 0.069 -0.0075 -0.007 0
HSetNx 0.855 0.001 0.0009 0
HStrLen 2.926 0.1714 0.1712 0
HVals 5.505 0.1884 0.1851 5600

For me, Variant looks minimal with the PR but I like to know your opinion. I will analyze why there is an additional allocation for HKeys, HGetAll, HVals commands

@Vijay-Nirmal
Copy link
Contributor Author

Vijay-Nirmal commented Dec 13, 2024 •
edited
Loading

I have to remove the AsEnumerable method to remove the addition allocation

This PR as of 0b965463eeab0bffc4cd1b09ee845de7f3380148

Method Params Mean Error StdDev Allocated
HSetDel ACL 71.355 us 0.8986 us 0.8405 us 16000 B
HExists ACL 6.656 us 0.0857 us 0.0801 us -
HGet ACL 5.668 us 0.0658 us 0.0615 us -
HGetAll ACL 5.624 us 0.1110 us 0.0927 us -
HIncrby ACL 8.037 us 0.1302 us 0.1218 us -
HIncrbyFloat ACL 9.284 us 0.1091 us 0.0967 us -
HKeys ACL 5.278 us 0.0854 us 0.0799 us -
HLen ACL 5.505 us 0.0404 us 0.0358 us -
HMGet ACL 6.234 us 0.0575 us 0.0480 us -
HMSet ACL 7.072 us 0.0270 us 0.0240 us -
HRandField ACL 7.479 us 0.0601 us 0.0562 us -
HScan ACL 6.351 us 0.0557 us 0.0521 us 776 B
HSetNx ACL 6.473 us 0.0688 us 0.0574 us -
HStrLen ACL 8.315 us 0.0800 us 0.0748 us -
HVals ACL 5.430 us 0.0664 us 0.0621 us -
HSetDel AOF 81.206 us 0.9149 us 0.8110 us 16000 B
HExists AOF 30.288 us 0.3712 us 0.3100 us 6400 B
HGet AOF 28.491 us 0.1386 us 0.1229 us 6400 B
HGetAll AOF 32.035 us 0.3467 us 0.3243 us 3200 B
HIncrby AOF 45.335 us 0.7194 us 0.6377 us 9600 B
HIncrbyFloat AOF 63.814 us 0.8268 us 0.7734 us 14400 B
HKeys AOF 30.770 us 0.3229 us 0.3020 us 3200 B
HLen AOF 26.871 us 0.2020 us 0.1791 us 3200 B
HMGet AOF 32.330 us 0.3322 us 0.2945 us 9600 B
HMSet AOF 50.428 us 0.5202 us 0.4612 us 16000 B
HRandField AOF 36.380 us 0.3982 us 0.3725 us 8800 B
HScan AOF 6.256 us 0.0510 us 0.0452 us 776 B
HSetNx AOF 39.430 us 0.5203 us 0.4867 us 9600 B
HStrLen AOF 29.398 us 0.3341 us 0.3125 us 6400 B
HVals AOF 30.651 us 0.3408 us 0.3188 us 3200 B
HSetDel None 69.603 us 0.7780 us 0.6897 us 16000 B
HExists None 28.505 us 0.1968 us 0.1840 us 6400 B
HGet None 28.940 us 0.2760 us 0.2582 us 6400 B
HGetAll None 32.346 us 0.3556 us 0.3152 us 3200 B
HIncrby None 38.459 us 0.3861 us 0.3611 us 9600 B
HIncrbyFloat None 57.310 us 0.9511 us 0.8897 us 14400 B
HKeys None 31.158 us 0.4847 us 0.4297 us 3200 B
HLen None 26.585 us 0.1706 us 0.1596 us 3200 B
HMGet None 31.758 us 0.4567 us 0.4272 us 9600 B
HMSet None 43.405 us 0.4953 us 0.4633 us 16000 B
HRandField None 37.259 us 0.3383 us 0.3164 us 8800 B
HScan None 6.346 us 0.0368 us 0.0344 us 776 B
HSetNx None 35.064 us 0.3838 us 0.3205 us 9600 B
HStrLen None 30.699 us 0.2534 us 0.2371 us 6400 B
HVals None 30.512 us 0.3009 us 0.2512 us 3200 B

Difference PR - Main

Method Params Mean Error StdDev Allocated
HSetDel ACL -0.048 0.1148 0.1073 0
HExists ACL -0.242 0.0213 0.023 0
HGet ACL -0.27 -0.0148 -0.0139 0
HGetAll ACL 0.409 0.0204 0.008 0
HIncrby ACL 0.451 0.0236 0.0221 0
HIncrbyFloat ACL 1.342 0.0322 0.0247 0
HKeys ACL -1.236 -0.0445 -0.0797 0
HLen ACL -0.251 -0.0509 -0.0496 0
HMGet ACL -0.31 -0.0728 -0.1214 0
HMSet ACL -0.926 -0.1314 -0.1164 0
HRandField ACL 0.692 0.0186 0.0174 0
HScan ACL 0.062 -0.0073 -0.0068 0
HSetNx ACL -0.378 0.0299 0.0229 0
HStrLen ACL 0.821 0.0087 0.0116 0
HVals ACL -0.393 0.0139 0.013 0
HSetDel AOF 2.563 -0.2433 -0.2724 0
HExists AOF 2.45 0.1107 0.0663 0
HGet AOF 0.483 -0.1229 -0.1217 0
HGetAll AOF 1.067 0.0432 0.0404 0
HIncrby AOF 1.641 0.269 0.2384 0
HIncrbyFloat AOF 1.88 0.0099 0.0093 0
HKeys AOF 0.287 -0.0787 -0.0737 0
HLen AOF 1.968 0.003 -0.007 0
HMGet AOF 0.139 0.098 0.0869 0
HMSet AOF 7.151 0.1283 0.0946 0
HRandField AOF 1.221 0.1556 0.1456 0
HScan AOF -0.094 0.0074 0.0044 0
HSetNx AOF 1.165 0.1047 0.1183 0
HStrLen AOF 1.221 0.0915 0.0975 0
HVals AOF 1.439 0.1884 0.1762 0
HSetDel None 1.565 0.1338 0.0871 0
HExists None -0.248 -0.0247 -0.0232 0
HGet None 2.136 0.1133 0.1061 0
HGetAll None 0.626 0.0764 0.0677 0
HIncrby None 1.157 0.0318 0.0297 0
HIncrbyFloat None 0.227 0.4261 0.3987 0
HKeys None 1.339 0.2615 0.2319 0
HLen None 2.137 -0.0986 -0.0922 0
HMGet None 0.363 0.082 0.0767 0
HMSet None 5.403 0.1399 0.1309 0
HRandField None 1.412 0.0475 0.0444 0
HScan None 0.044 -0.0194 -0.0182 0
HSetNx None 2.502 -0.0124 -0.0501 0
HStrLen None 2.8 0.0335 0.0422 0
HVals None 1.976 0.1206 0.0914 0

For me, HMSet is the only one that looks outstanding.

Update: Not sure what's going on in HMSet commands. Even after reverting everything back in HMSet commands flow, the performance is still slower than the main. But changing other parts of the code affects its performance like using Count() instead of hash.Count in HashLength method affect the performance of HMSet commands. Even if I just have return hash.Count inside the Count() method, still the performance is affected. There is other weird behaviour with HMSet command's performance.

vazois
vazois requested changes Dec 14, 2024
View reviewed changes
Copy link
Contributor

@vazois vazois left a comment

Choose a reason for hiding this comment

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

Great work. Please find my comments below

website/docs/commands/data-structures.md Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObject.cs Show resolved Hide resolved
libs/server/Objects/Hash/HashObject.cs Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Objects/Hash/HashObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Storage/Session/Common/MemoryUtils.cs Show resolved Hide resolved
libs/server/Storage/Session/ObjectStore/HashOps.cs Outdated Show resolved Hide resolved
libs/server/Storage/Session/ObjectStore/HashOps.cs Outdated Show resolved Hide resolved
libs/server/StoreWrapper.cs Outdated Show resolved Hide resolved
badrishc
badrishc requested changes Dec 14, 2024
View reviewed changes
Copy link
Contributor

@badrishc badrishc left a comment

Choose a reason for hiding this comment

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

See comments

libs/host/Configuration/Options.cs Outdated Show resolved Hide resolved
libs/resources/RespCommandsInfo.json Show resolved Hide resolved
libs/server/Storage/Session/ObjectStore/HashOps.cs Outdated Show resolved Hide resolved
@Vijay-Nirmal
Copy link
Contributor Author

Fixed all review comments except the one where we need to find the size of the new objects

@Vijay-Nirmal
Copy link
Contributor Author

Please rereview this PR, closed all outstanding items from my end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yrajas yrajas yrajas left review comments

@vazois vazois vazois approved these changes

@badrishc badrishc badrishc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add HEXPIRE
4 participants
@Vijay-Nirmal @badrishc @yrajas @vazois