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

Added BDN for all Sorted Set commands #991

Merged
merged 8 commits into from
Feb 7, 2025

Conversation

Vijay-Nirmal
Copy link
Contributor

This pull request introduces a new benchmark for all Sorted Set commands as per #879 (comment).

  • ZADD
  • ZREM
  • ZCARD
  • ZCOUNT
  • ZDIFF
  • ZDIFFSTORE
  • ZINCRBY
  • ZINTER
  • ZINTERCARD
  • ZINTERSTORE
  • ZLEXCOUNT
  • ZMPOP
  • ZMSCORE
  • ZPOPMAX
  • ZPOPMIN
  • ZRANDMEMBER
  • ZRANGE
  • ZRANGESTORE
  • ZRANK
  • ZREMRANGEBYLEX
  • ZREMRANGEBYRANK
  • ZREMRANGEBYSCORE
  • ZREVRANK
  • ZSCAN
  • ZSCORE
  • ZUNION
  • ZUNIONSTORE

@Vijay-Nirmal
Copy link
Contributor Author

@darrenge @badrishc Can "Garnet CI BDN.benchmark" action be triggered from this branch to find the expected run time on the agent to update the details in BDN_Benchmark_Config.json file?

@badrishc
Copy link
Contributor

badrishc commented Feb 3, 2025

@darrenge @badrishc Can "Garnet CI BDN.benchmark" action be triggered from this branch to find the expected run time on the agent to update the details in BDN_Benchmark_Config.json file?

Triggered here: https://github.com/microsoft/garnet/actions/runs/13105237393
Results will also post as comments on this commit: 0e12cc3

@Vijay-Nirmal
Copy link
Contributor Author

@badrishc I don't see SortedSetOperations in the benchmark result. Didn't spend time to analyze why but I assumed it because I didn't update BDN_Benchmark_Config.json. Now I have updated, can you retrigger it?

@badrishc
Copy link
Contributor

badrishc commented Feb 3, 2025

@badrishc I don't see SortedSetOperations in the benchmark result. Didn't spend time to analyze why but I assumed it because I didn't update BDN_Benchmark_Config.json. Now I have updated, can you retrigger it?

I thought there was an attachment related to sorted sets, at the bottom of the actions page.
Anyway, retriggered it at https://github.com/microsoft/garnet/actions/runs/13120147877

@darrenge
Copy link
Contributor

darrenge commented Feb 3, 2025

Looks like Badrish has answered your questions. If you have more, let me know.

Copy link
Contributor

@darrenge darrenge left a comment

Choose a reason for hiding this comment

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

  1. in test\BDNPerfTests\run_bdnperftest.ps1 the size of objects you are accessing is more than 70 so change line to $expectedResultsArray = New-Object 'string[,]' 100, 3

  2. You will need to update the BDN_Benchmark_Config.json file. You can run BDN manually or run this ps1 which is ran in the BDN pipeline:
    .\test\BDNPerfTests> .\run_bdnperftest.ps1 Operations.SortedSetOperations

@darrenge
Copy link
Contributor

darrenge commented Feb 6, 2025

@Vijay-Nirmal From the looks of it, it is ready other than the two changes I requested.

@Vijay-Nirmal
Copy link
Contributor Author

@darrenge Fixed it

@darrenge
Copy link
Contributor

darrenge commented Feb 6, 2025

Trying it out locally, but I see branch Vijay-Nirmal/BDN/sorted-set does NOT have these changes. The pr/991 branch has today's changes so I was able to get things locally. However, since Vijay-Nirmal... branch is not up to date, I can't actually run the BDN pipeline on it. I tried: git branch --set-upstream-to=origin/Vijay-Nirmal/BDN/sorted-set Vijay-Nirmal/BDN/sorted-set @Vijay-Nirmal - can you get your original branch (Vijay-Nirmal/BDN/sorted-set ) up to date?

Copy link
Contributor

@darrenge darrenge left a comment

Choose a reason for hiding this comment

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

I ran this locally on my WIndows machine and there were four places where the expected is lower than the actual. If you are testing on a linux machine, then these might differ and you should take the higher value. The ones I saw failed:
ZInter AOF The BDN benchmark found Allocated value (123200) is above the acceptable threshold of 37840 (Expected value 34400 + 10%)
ZUnion AOF The BDN benchmark found Allocated value (129600) is above the acceptable threshold of 37840 (Expected value 34400 + 10%)
ZInter None The BDN benchmark found Allocated value (79200) is above the acceptable threshold of 37840 (Expected value 34400 + 10%)
ZUnion None The BDN benchmark found Allocated value (84800) is above the acceptable threshold of 37840 (Expected value 34400 + 10%)

@badrishc
Copy link
Contributor

badrishc commented Feb 6, 2025

Trying it out locally, but I see branch Vijay-Nirmal/BDN/sorted-set does NOT have these changes. The pr/991 branch has today's changes so I was able to get things locally. However, since Vijay-Nirmal... branch is not up to date, I can't actually run the BDN pipeline on it. I tried: git branch --set-upstream-to=origin/Vijay-Nirmal/BDN/sorted-set Vijay-Nirmal/BDN/sorted-set @Vijay-Nirmal - can you get your original branch (Vijay-Nirmal/BDN/sorted-set ) up to date?

Vijay-Nirmal/BDN/sorted-set is a mirror i created for the BDN purpose. I have updated it to reflect the latest from Vijay's branch.

@Vijay-Nirmal
Copy link
Contributor Author

@darrenge Fixed the issue. Issue was with run_bdnperftest.ps1 file which have issues when one method name is also part of another method's name

@Vijay-Nirmal Vijay-Nirmal requested a review from darrenge February 7, 2025 08:28
Copy link
Contributor

@darrenge darrenge left a comment

Choose a reason for hiding this comment

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

Looked over the code changes and ran the BDN GH Action and everything looks good.

@darrenge darrenge merged commit 86c544b into microsoft:main Feb 7, 2025
15 checks passed
@Vijay-Nirmal Vijay-Nirmal deleted the BDN/sorted-set branch February 7, 2025 19:27
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.

3 participants