-
Notifications
You must be signed in to change notification settings - Fork 83
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-5358 Improve CompoundIndexes.yml workload #1214
Conversation
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.
It doesn't look like you have any tasks in the patch yet.
@@ -278,10 +284,70 @@ Actors: | |||
collection: *coll | |||
query: &query { | |||
Filter: { | |||
tenantId: {$eq : 0}, | |||
tenantId: {$eq : 1}, |
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.
I was wondering why the tenantId changed but you are still picking the tenantId.
Is the overall effect of the query change to roughly retain the selectivity of the original?
You have answered this in the description.
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.
This particular change was because I wanted to have a constant above which determines the number if unique tenantIds
, which in this case is 3. The {distribution: uniform, min: x, max: y}
expects the min and max bounds to be inclusive, so it's easiest to express using a min of 1 rather than 0. That meant I had to update the filter, since we used to generate tenant ids starting at 0.
Oops, thanks for the reminder. I just reconfigured the patch to run the |
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.
The Description is out of step with the test now.
Specifically the 'predicates a small number of fields'. Can you correct that and add something about the selectivity?
The workload is being executed on the following ARM variants:
- Classic Query Engine Standalone ARM AWS 2023-11
- SBE Standalone ARM AWS 2023-11
These have very similar performance. Do these variants correspond to 'classic' and 'SBE' or 'classic' and 'classic planner + SBE execution'? And what does this mean for the following statement?
We expect classic to have better latency and throughput than SBE on this workload,
and we expect the combination of classic planner + SBE execution (PM-3591) to perform about as well as classic.
Also: theres a fairly large drop in performance ... we'll should make sure the build barons are aware of it beforehand.
I didn't fix this originally because this would conflict with @dpercy's work in #1213. I think it makes more sense to fix the description as part of this patch though, so I've pushed a new commit which does so. @dpercy heads up that this will conflict with your patch.
Regarding this:
I cleaned up the description to not focus on the differences between the three configurations -- this is an artifact of the context under which the workload was originally developed and seems less relevant as we continue to run this workload going forward. I think we should do similar cleanup across all of the multi-planning workloads as part of #1213. Since both SBE and classic always use the classic multi-planner now, and this is a multi-planning workload, it's not surprising that these two configurations are exhibiting similar performance. I've re-requested review, please take another look! |
Oh, one other thing. @dpercy @jimoleary your comment above made me realize that
Should we file and schedule a ticket to make sure that the multi-planning genny workloads are actually running on all-feature-flags variants in the master branch? |
Yeah I think so ... we could fix it in #1213 but this PR is fairly targetted and it would be nice to fix this with a ticket. |
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.
LGTM with a new ticket for the autoruns and informing the barons of the change in performance.
Thanks for the reviews! The Evergreen patch failed to due Here's a performance analyzer link for reference: https://performance-analyzer.server-tig.prod.corp.mongodb.com/perf-analyzer-viz/?comparison_id=0af281c3-dcd4-4d70-b441-8bbf4d6b42a2 I've alerted #performance-build-baroning in this thread: https://mongodb.slack.com/archives/C8CT98KL4/p1715274831473279 I filed DEVPROD-7308 about fixing the build variants on which these workloads run and will look for an assignee. |
I had to merge with the linting changes recently made in 414f74e#diff-5dd88d22cd5fe6b47267656c0c3d7c1bb1096e1a2f6bc975bce19d57977f57cb. This passes the linter locally now and I will let Evergreen re-run. I've chosen to keep the more compact formatting that the workload previously had prior to 414f74e#diff-5dd88d22cd5fe6b47267656c0c3d7c1bb1096e1a2f6bc975bce19d57977f57cb since the linter still seems to be happy with it and I find it to be a bit more readable. Auto-merge is enabled, so this will merge if Evergreen remains happy. |
Jira Ticket: PERF-5358
Whats Changed
There are three related changes:
Patch Testing Results
https://spruce.mongodb.com/version/663bb9ac889ffa0007942ecb/tasks