Try to simplify logging a bit #3
Merged
Conversation
This file contains 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
There is lots of code looking like this:
utils::log(&format!("error powering off: {}", err));
Simplify this by adding a log!() macro that hides the format!() dance
and can be used directly like format!().
Signed-off-by: Uli Schlachter <psychon@znc.in>
Before this commit, the caller of log_impl() (= the macro log!())
already did a memory allocation for constructing a String containing the
message to log. This was then borrowed when calling log_impl(). This
function then did another memory allocation plus copy to prepend
"virtme-ng-init:" to the message. Then, if /dev/kmsg exists, another
allocation is done to add a kernel log level tag at the beginning and a
newline at the end.
This commit starts by changing log!() from format!() to format_args!()
which constructs an instance of std::fmt::Arguments. This is just a
specification of how to do the formatting, but nothing was actually done
yet. This is then used in log_impl() to directly construct a string with
"virtme-ng-init:" prepended to the message to log.
Since I wanted to avoid memory allocations, this then replaces
.trim_end_matches('\n') with a manual loop. That way, we still have a
String and do not switch over to a &str for the following code.
Additionally, the "is the message to log empty?"-check was reformulated
to "was there nothing appended to my prefix?". This makes things
slightly more unreadable, but I couldn't find a better way to check
whether the Arguments instance is empty.
There is a slight change in behaviour here: Previously, log!("\n") would
log an empty line since the code first checked if the message is empty
and only afterwards trimmed trailing newlines. This is now done the
other way around.
Finally, when writing to /dev/kmsg, the String that we already have is
mutated in places instead of allocating another one and copying all data
to it.
Why? Just because. I doubt that this will have any measurable
performance benefits, but somehow it felt like the right thing to do.
Signed-off-by: Uli Schlachter <psychon@znc.in>
write() is allowed to write only partial data. Good luck debugging why some part of the log message is missing. This commit changes the logging to use write_all() instead, which ensures everything is written. This might end up doing multiple writes to /dev/kmsg, which would turn into multiple log records. I think that is still better than silently discarding some data. Signed-off-by: Uli Schlachter <psychon@znc.in>
The logging code already does trim_end_matches('\n') so we do not have
to the that in the caller, too.
Signed-off-by: Uli Schlachter <psychon@znc.in>
|
@psychon good stuff! I didn't notice any measurable performance boost, but it all makes sense to me and the code looks much more readable now, therefore applied. Thanks! |
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.
In my previous PR, I wrote:
Turns out this is not as complicated as I first thought! I can just introduce a macro that hides the
format!inlog(&format!(....)).Next, I looked at the standard library. If we press
sourceon the docs of theprint!()macro, we learn that this is based onstd::format_args!(). Its documentation indicates that this is some kind of container for the formatted string and can be used inDisplaycontext to really construct the formatted string. I then went nuts and optimised away the threeStringallocations in the logging implementation into a single allocation. This definitely did not make the code more readable.As with the last PR: Feel free to do whatever you want with that. Especially the needless logging optimisation can be taken out if you do not like it. Just tell me so and I'll do it.