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

Optimization for Logging RAM usage #724

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nhelou
Copy link

@nhelou nhelou commented Jun 19, 2024

This change is an improvement to the message class of the internal logger. Instead of checking if logging is active in the destructor this is now done in the constructor. If logging for the specific level is deactivated no data is written to the internal std::stringstream and thus the performace is improved.

…gging is activated, otherwise does not allocat any data.
#endif
#endif // USE_DLT
} catch (const std::exception& e) {
std::cout << "\nVSIP: Error destroying message class: " << e.what() << '\n';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't cerr be more appropriate?

return active_;
}

inline std::string str() const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if a string_view would be more appropriate here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure, std::stringstream::str() constructs a new string anyway and wouldn't std::string_view then point to a temporaray object? Maybe I am wrong ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be entirely wrong, but I think what happens here that a new std::string is constructed (with allocation) here when buffer_.str() is involved, and then the string stream copies the contents of this into the new string it is constructing. If this were a std::string_stream, we may save on one allocation. If I'm right this could be considerable on platforms like QNX.

I suppose the only way to know would be to test it out. I can try to set up something small on godbolt.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set up a minimal example in godbolt and found exactly where I was confused. I didn't see that data_ was a stringstream. So there's no savings to be had it seems.

That said, C++20 has a stringstream::view that returns a string_view, unfortunately nothing appears to support it yet, so maybe one day...

Copy link
Contributor

@kheaactua kheaactua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. I haven't build/run it locally though. If this gets approved I'd also like to conflate it with my logger PR where all the severity switch statements are moved into functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants