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

Perf Improvements to Custom Commands' Execution Path #940

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

Conversation

TalZaccai
Copy link
Contributor

@TalZaccai TalZaccai commented Jan 21, 2025

This PR introduces some perf improvements to the execution path of custom commands:

  • Unlatched access to custom commands' data structures in CustomCommandManager
  • Eliminating expensive string-based RespCommandInfo access for arity check by adding arity property to commands classes.

Benchmarking results:

Current branch:

CustomOperations:

Method Params Mean Error StdDev Allocated
CustomRawStringCommand None 39.04 us 0.657 us 0.549 us -
CustomObjectCommand None 147.69 us 2.832 us 2.511 us 24000 B
CustomTransaction None 91.46 us 1.817 us 2.092 us -
CustomProcedure None 73.25 us 0.966 us 0.807 us -

ModuleOperations:

Method Params Mean Error StdDev Gen0 Allocated
ModuleNoOpRawStringReadCommand None 36.99 us 0.707 us 1.851 us - -
ModuleNoOpRawStringRmwCommand None 45.56 us 0.879 us 1.144 us - -
ModuleNoOpObjRmwCommand None 68.98 us 1.374 us 2.477 us - 8800 B
ModuleNoOpObjReadCommand None 55.09 us 0.936 us 1.078 us - 5600 B
ModuleNoOpProc None 29.64 us 0.308 us 0.288 us - -
ModuleNoOpTxn None 28.20 us 0.346 us 0.307 us - -
ModuleJsonGetCommand None 121.39 us 2.409 us 5.630 us - 72800 B
ModuleJsonSetCommand None 218.26 us 4.297 us 7.060 us 0.7324 228800 B

main:

CustomOperations:

Method Params Mean Error StdDev Median Allocated
CustomRawStringCommand None 46.06 us 0.692 us 0.824 us 45.70 us -
CustomObjectCommand None 175.01 us 4.435 us 12.216 us 170.85 us 24000 B
CustomTransaction None 98.49 us 1.959 us 2.932 us 98.16 us -
CustomProcedure None 83.08 us 1.629 us 2.938 us 82.53 us -

ModuleOperations:

Method Params Mean Error StdDev Gen0 Allocated
ModuleNoOpRawStringReadCommand None 46.36 us 0.927 us 1.624 us - -
ModuleNoOpRawStringRmwCommand None 54.67 us 1.083 us 0.960 us - -
ModuleNoOpObjRmwCommand None 84.71 us 1.627 us 4.400 us - 8800 B
ModuleNoOpObjReadCommand None 69.37 us 1.373 us 1.527 us - 5600 B
ModuleNoOpProc None 39.89 us 0.794 us 1.259 us - -
ModuleNoOpTxn None 34.98 us 0.649 us 1.136 us - -
ModuleJsonGetCommand None 131.93 us 2.474 us 4.884 us - 72800 B
ModuleJsonSetCommand None 235.78 us 3.061 us 2.863 us 0.4883 228801 B

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.

looking good, thanks!

@TalZaccai TalZaccai requested a review from badrishc January 25, 2025 01:49
sessionTransactionProcMap = new ExpandableMap<CustomTransactionProcedureWithArity>(CustomCommandManager.MinMapSize, 0, byte.MaxValue);
sessionCustomProcMap = new ExpandableMap<CustomProcedure>(CustomCommandManager.MinMapSize, 0, byte.MaxValue);

sessionTransactionProcMap = new ExpandableMap<(CustomTransactionProcedure, int)>(MinMapSize, 0, byte.MaxValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not start the expandable map at 0 so that we dont pay the cost of memory in the common case that there are no custom commands? then then grow as 1,2,4, etc.

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.

fix memory use by default.

private static readonly int MaxCustomRawStringCommands = 256;

// Initial size of expandable maps
private static readonly int MinMapSize = 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not start the expandable map at 0 so that we dont pay the cost of memory in the common case that there are no custom commands? then then grow as 1,2,4, etc. (for CCM and CCMS)

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.

2 participants