feat(clp-s::log_converter): Re-use the same log_surgeon::BufferParser for converting each file.#2284
Conversation
… LogConverter::convert_file.
WalkthroughLogConverter is refactored to centralize ChangesLogConverter Parser Factory Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
davidlion
left a comment
There was a problem hiding this comment.
lgtm other than the optional nit.
| log_surgeon::BufferParser m_parser; | ||
| ystdlib::containers::Array<char> m_buffer; | ||
| size_t m_num_bytes_buffered{}; | ||
| size_t m_parser_offset{}; |
There was a problem hiding this comment.
I don't think clp follows the alphabetical rule from https://docs.yscope.com/dev-guide/contrib-guides-general.html very strictly, but technically this does violate it.
I'll leave it up to you if you want to fix it.
There was a problem hiding this comment.
I think I'll leave this one as is since it leaves the members grouped together in a more logical way, but will try to keep this rule in mind.
Description
This PR modifies the
LogConverterclass to have it own an instance oflog_surgeon::BufferParserthat is created during initialization, and re-used to convert each input file. The previous implementation created a newlog_surgeon::BufferParseron each invocation ofconvert_file, which adds quite a bit of overhead when converting many small files.Conversion of the
hive-24hrdataset goes from about 10 minutes down to about 2 minutes on an old xeon server I tested this change on.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Release Notes