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

dnf5: "check" command - optimize, don't use rpmcli #1056

Merged

Conversation

jrohel
Copy link
Contributor

@jrohel jrohel commented Nov 30, 2023

It uses a loop to go through the rpmdb database instead of using the rpmcliVerify and rpmcliQuery functions (which then called back our code).

Why this PR:

  • The original version uses the rpmcliVerify and rpmcliQuery functions from the librpm library. These functions, from C library, go through the rpmdb database and call back a C++ function from our code for each element. Our C++ function may throw an exception. The result may work, but it's generally better to avoid this situation (passing C++ exceptions through the C library.). In the new version, our code goes through the database and C callbacks are not used.
  • librpm does not pass "user" data in callbacks. Therefore, they were passed through global variables (ok, they are in an anonymous namespace, available only in the given object file) but the code is not reentrant. The new version also uses global variables, but this is no longer necessary, we are not limited by callbacks in librpm. If we need to get rid of global variables (move the code to libdnf, make it reentrant, ...) it is now an easy change.
  • The new code is simpler.
  • The processor will do a few less instructions. But overall, the difference in speed will probably be negligible. That's not why I did this PR.

A loop to go through the rpmdb database instead of using the
`rpmcliVerify` and `rpmcliQuery` functions (which then called back our
code).
@jrohel jrohel force-pushed the dnf5/command_check.librpm2 branch from e21f4bb to 28e2e93 Compare December 4, 2023 10:30
@jrohel jrohel changed the title dnf5: "check" command - optimize, don't use rpmcli and use one transaction with all packages dnf5: "check" command - optimize, don't use rpmcli Dec 4, 2023
@j-mracek j-mracek self-assigned this Dec 6, 2023
@j-mracek
Copy link
Contributor

j-mracek commented Dec 7, 2023

May I ask you to add performance comparison to commit message?

@j-mracek
Copy link
Contributor

j-mracek commented Dec 7, 2023

I am sorry, I am not sure what it optimaze, because it requires the same time. Is it only a code simplification?

@jrohel
Copy link
Contributor Author

jrohel commented Dec 7, 2023

@j-mracek I added an explanation to the PR description.

@j-mracek
Copy link
Contributor

LGTM

@j-mracek j-mracek added this pull request to the merge queue Dec 11, 2023
Merged via the queue into rpm-software-management:main with commit 4c05205 Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants