Logging to help debugging #107
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We should try to track down the perf, but the logging is important and it is worth the change. |
Eh2406
reviewed
Aug 17, 2021
src/internal/partial_solution.rs
Outdated
| .iter() | ||
| .map(|(p, pa)| format!("{}: {}", p, pa)) | ||
| .collect(); | ||
| let assignments: Vec<_> = assignments.into_iter().collect(); |
There was a problem hiding this comment.
I think it would be clearer to collect into a vec and then call sort.
Eh2406
reviewed
Aug 17, 2021
Cargo.toml
Outdated
| @@ -23,12 +23,14 @@ include = ["Cargo.toml", "LICENSE", "README.md", "src/**", "tests/**", "examples | |||
| thiserror = "1.0" | |||
| rustc-hash = "1.1.0" | |||
| serde = { version = "1.0", features = ["derive"], optional = true } | |||
| log = { version = "0.4.14", default-features = false } # for debug logs in tests | |||
There was a problem hiding this comment.
No objection, just wondering:
What features are we trying to opted out of? Why default-features = false?
|
I did not see any pref impact. So I addressed my nits, and made clippy happy. I will merge when CI is green. |
Eh2406
added a commit
that referenced
this pull request
Aug 21, 2021
* feat: add logging to help debugging * debug: impl Display for partial solution * fix cherry picking * Fix display of assignments in partial_solution * debug: nits Co-authored-by: Jacob Finkelman <[email protected]>
zanieb
pushed a commit
to astral-sh/pubgrub
that referenced
this pull request
Nov 8, 2023
* feat: add logging to help debugging * debug: impl Display for partial solution * fix cherry picking * Fix display of assignments in partial_solution * debug: nits Co-authored-by: Jacob Finkelman <[email protected]>
This was referenced Nov 15, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From my experiments with the interval design, I realized that being able to log the important steps of the solver was quite helpful to locate the origin of bugs. In this PR, I've introduced the
logcrate and added some calls tolog::info!()in the solver. I've also addedenv_loggerto the dev dependencies to be able to display logs in failing tests.After running the benchmarks, I think this has an impact on the zuse bench where I saw roughly 10% slower performances. I did not notice performance changes on the other benchmarks. I wonder what it is though that could impact performances. I thought the
logcrate did not impact performances if it is not instantiated with a logger. Could this be because the fewDisplayimplementations that I added on some internal types?