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

PERF-5440 Fix created indices for ascending sort key queries in TimeSeriesLastpoint.yml workload #1219

Merged
merged 2 commits into from May 22, 2024
Merged

PERF-5440 Fix created indices for ascending sort key queries in TimeSeriesLastpoint.yml workload #1219

merged 2 commits into from May 22, 2024

Conversation

yun-soo
Copy link
Contributor

@yun-soo yun-soo commented May 11, 2024 •
edited

Jira Ticket: PERF-5440

Whats Changed

  • Fix wrong index creation for ascending time sort key queries
  • Repeat optimized last point queries100 times instead of 10 to be able to collect on-cpu profiling
  • Use CrudActor instead of RunCommand to execute the aggregates
  • Use OnlyActiveInPhases

Patch Testing Results

https://spruce.mongodb.com/version/663feee5e0ec0400072fd1cb/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

@yun-soo yun-soo requested a review from a team as a code owner May 11, 2024 22:16
@yun-soo yun-soo requested a review from brad-devlugt May 11, 2024 22:16
@yun-soo yun-soo mentioned this pull request May 11, 2024
Open
jawwadasghar
jawwadasghar requested changes May 21, 2024
View reviewed changes
Copy link
Contributor

@jawwadasghar jawwadasghar left a comment

Choose a reason for hiding this comment

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

Hey Yoonsoo, this looks great!

I left a few comments in #1218 which apply here.
Namely,
For AutoRun conditions

  • can we remove the standalone conditions?
  • should the branch conditional be "$gte: v7.0"?

It might be clearer to define active phases for the QuiesceActor within the workload file rather than the phases file

yun-soo
yun-soo commented May 22, 2024
View reviewed changes
Copy link
Contributor Author

@yun-soo yun-soo left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for review, @jawwadasghar!

can we remove the standalone conditions?

Definitely, removed.

should the branch conditional be "$gte: v7.0"?

This workload & related optimization has existed since v5.3. The reason why I set branch condition for the new workload to $gte: v7.0 is because it's a new one and we care about its perf against v7.0. It's not a part of high value workload. I changed this to $gte: v5.3

It might be clearer to define active phases for the QuiesceActor within the workload file rather than the phases file

done.

@yun-soo yun-soo requested review from jawwadasghar and removed request for brad-devlugt May 22, 2024 15:03
jawwadasghar
jawwadasghar approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@jawwadasghar jawwadasghar left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks Yun-soo!

@yun-soo yun-soo added this pull request to the merge queue May 22, 2024
Merged via the queue into master with commit 633ca77 May 22, 2024
11 checks passed
@yun-soo yun-soo deleted the yoonsoo.kim/PERF-5440 branch May 22, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jawwadasghar jawwadasghar jawwadasghar 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.

None yet
2 participants
@yun-soo @jawwadasghar