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

Firehose Log: Performance + Refactor #43

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

dgmcdona
Copy link
Contributor

This PR does some refactoring in firehose_log.rs that should provide some performance gains by eliminating allocation of several Vecs throughout the file. It also simplifies some nom parsing, where data was first being extracted with take before being handed off to the integer parsers.

Replaces `Vec` uses with static arrays, declares variables as `const`,
and renames those variables to all caps.
This removes unneeded calls to `take(size_of::<>())`.
This reduces the overall number of allocations by removing two vecs of
intermediate string allocations, and instead stores the raw numbers,
waiting to render them into strings until the final output vec is
constructed.
@jrouaix
Copy link
Contributor

jrouaix commented Dec 10, 2024

Hi @dgmcdona, I see you also have refactor ambitions on this lib.

If you want you can pick some from this abandonned PR where I have been "a little too much" : https://github.com/mandiant/macos-UnifiedLogs/pull/22/files

Glad to have another nom lover on duty !

@dgmcdona
Copy link
Contributor Author

Hi @dgmcdona, I see you also have refactor ambitions on this lib.

If you want you can pick some from this abandonned PR where I have been "a little too much" : https://github.com/mandiant/macos-UnifiedLogs/pull/22/files

Glad to have another nom lover on duty !

Thanks, and likewise! I'll check out your changes and see if there is anything I should pull in with these.

@puffyCid
Copy link
Collaborator

looks good thanks!

@puffyCid puffyCid merged commit f91018c into mandiant:main Dec 16, 2024
4 checks passed
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.

3 participants