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

Don't make calls to the database in for loops #530

Closed
jyn514 opened this issue Dec 19, 2019 · 5 comments
Closed

Don't make calls to the database in for loops #530

jyn514 opened this issue Dec 19, 2019 · 5 comments
Assignees
Labels
E-easy Effort: Should be easy to implement and would make a good first PR P-low Low priority issues

Comments

@jyn514
Copy link
Member

jyn514 commented Dec 19, 2019

This is a low-priority issue, we should fix #379 and #343 first.

This came up while reviewing #529. Offenders:

@jyn514 jyn514 added the minor label Dec 22, 2019
@jyn514 jyn514 added E-easy Effort: Should be easy to implement and would make a good first PR P-low Low priority issues and removed minor labels Jun 27, 2020
@jyn514
Copy link
Member Author

jyn514 commented Apr 17, 2021

@syphar are you interested in taking a look at this? I don't want to burden you with too much work, but it seems right up your alley :)

@syphar
Copy link
Member

syphar commented Apr 18, 2021

It's definitely my alley :)

Actually I'm not sure why I already saw and fixed it when I looked through all issues :)

@syphar
Copy link
Member

syphar commented Apr 18, 2021

on current master, it looks like the remaining fixes are:

  • update_owners_in_database
  • add_keywords_into_database
  • add_compression_into_database

I'll start with these, and other small optimizations I find in db::add_package.

Can you elaborate where you see an issue with add_path_into_database or add_package_into_database?

@jyn514
Copy link
Member Author

jyn514 commented Apr 18, 2021

add_path_into_database is very different now, most of it is in the storage module. I don't see anything in storage that looks bad off the top of my head (and performance doesn't matter for the test database anyway, only for S3).

I don't remember what I was thinking about add_package_into_database. Maybe I wanted to combine everything into a database transaction? That's already tracked at #1071.

@syphar
Copy link
Member

syphar commented Apr 18, 2021

I'll leave add_compression_into_database as it is, it's only one record in all our cases.

syphar added a commit to syphar/docs.rs that referenced this issue Apr 18, 2021
syphar added a commit to syphar/docs.rs that referenced this issue Apr 18, 2021
syphar added a commit to syphar/docs.rs that referenced this issue Apr 18, 2021
syphar added a commit to syphar/docs.rs that referenced this issue Apr 18, 2021
syphar added a commit to syphar/docs.rs that referenced this issue Apr 18, 2021
@jyn514 jyn514 closed this as completed in b4fc5ab Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Effort: Should be easy to implement and would make a good first PR P-low Low priority issues
Projects
None yet
Development

No branches or pull requests

2 participants