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

Performance updates and misc. #2264

Merged
merged 26 commits into from Mar 30, 2023
Merged

Performance updates and misc. #2264

merged 26 commits into from Mar 30, 2023

Conversation

sgoggins
Copy link
Member

Description

  • I think we should check the number of cores and use that information for setting parallel processes in the next release. My heuristic was, on a 64 CPU server, I set it to 50. This is working well. I think ~14 processes running in parallel less that those on the hardware is optimal. We have to check for "CPU", NOT "Cores" because the core checking does not work on OSX. So, we can go with ~14 less than the CPUS available for safety, or a default of 14. Whichever is greater.

We could also simply check the OS, and if its a linux flavor, evaluate cores.

@sgoggins
Copy link
Member Author

There is also a patch for the commits table here, setting the affiliation data to NULL instead of a string "NULL". The updates on the string "NULL" are very expensive.

Copy link
Contributor

@IsaacMilarky IsaacMilarky left a comment

Choose a reason for hiding this comment

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

This mostly looks good although I would change the repo scheduling to be more balanced now that we have a lot more processes to use and don't have to have as many messages in the queue.

@ABrain7710
Copy link
Contributor

I agree with @IsaacMilarky comment. Maybe like 40 processes for core and 15 for secondary? This would about triple the number of processes each worker

sgoggins and others added 5 commits March 24, 2023 15:32
…sure adequate vacuuming.

Signed-off-by: Sean Goggins <s@goggins.com>
Signed-off-by: Sean Goggins <s@goggins.com>
Signed-off-by: Sean Goggins <s@goggins.com>
Signed-off-by: Sean Goggins <s@goggins.com>
Signed-off-by: Sean Goggins <outdoors@acm.org>
Copy link
Contributor

@IsaacMilarky IsaacMilarky left a comment

Choose a reason for hiding this comment

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

I would set max_repo to 30 for start_primary_collection and I would set max_repo to 30 for start_facade_collection. Otherwise LGTM :)

augur/tasks/start_tasks.py Show resolved Hide resolved
sgoggins and others added 6 commits March 28, 2023 04:59
…need the update. If its not, which is usually the case, especially on collections 2 to n, we don't need to update it again.

The update to the foreign key to defer the dependency requirement which will accelerate initial inserts.
Signed-off-by: Sean Goggins <s@goggins.com>
Signed-off-by: Sean Goggins <outdoors@acm.org>
Signed-off-by: Sean Goggins <outdoors@acm.org>
Copy link
Contributor

@IsaacMilarky IsaacMilarky left a comment

Choose a reason for hiding this comment

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

I still don't think we should schedule more repos than processes we have available.

augur/application/cli/backend.py Outdated Show resolved Hide resolved
augur/tasks/github/facade_github/tasks.py Show resolved Hide resolved
augur/tasks/start_tasks.py Show resolved Hide resolved
@sgoggins sgoggins added the add-feature Adds new features label Mar 30, 2023
Signed-off-by: Sean Goggins <s@goggins.com>
@sgoggins
Copy link
Member Author

I went with 30 for primary and 20 for secondary @ABrain7710 because secondary takes so much longer and core is always waiting for it to finish.

@sgoggins sgoggins merged commit ac6856c into dev Mar 30, 2023
1 check passed
@sgoggins sgoggins deleted the spg-speed-test branch April 20, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add-feature Adds new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants