Closed dfreese closed 1 year ago
Thanks for the PR. This could have been probably better split in two commits, but I'm fine reviewing/merging it as a whole too.
The change on Priority
seems good, but I'm going to take a bit more time to reason about the extend()
change as it seems a bit convoluted.
Out of curiosity, did you see this hotspot showing up on some profiling/tracing session or are you just diving through the codebase?
Out of curiosity, did you see this hotspot showing up on some profiling/tracing session or are you just diving through the codebase?
I don't have benchmarks for this, but I have seen similar make a difference for logging-related that can be called quite often.
With the smaller scope of changes here (Priority
&str only), I'm inclined to just accept it and push the extend()
changes/discussion to another PR.
@swsnr sounds fine?
I've one question left about the numeric_level function, but other than that I like the current changes. The explicit reserve is a much less invasive change, has a clear benefit, and makes the intention of avoiding allocations explicit. @dfreese Nice change, thank you 🙏🏿
This removes the, match. Since rust does not have a small string optimization, this avoids an allocation for each call to
to_string()
call on Priority by adding a private member functionnumeric_level()
. I've implemented thenumeric_level()
as a private member function to avoid adding to the API surface of Priority. The provided test ensures that the function used internally and the representation publicly, via Fromjournal_send
.This also adds a reserve call prior to adding data to the vector to avoid multiple allocations when extend and push are called, since the length of the data being added is fully known.