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

Move preprocess into iterator #26

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

bovine3dom
Copy link

Disclaimer: I don't "get" Rust so a lot of what I do here might be mad. I probably listen to the compiler a little too much.

To my surprise, the PR does work.

Summary of changes:

  • I've moved most of the loop body in preprocess_rec out into the line_muncher function
    • I've made things mutable where it seemed prudent
  • I made an iterator, PreprocessHolder, that calls the line_muncher function and keeps track of the current line
  • I made preprocess_rec use that iterator

Motivation: BrettMayson/HEMTT#57

Even on a large config, it hasn't degraded performance:

bash-5.0$ time ~/projects/armake2/target/debug/armake2 preprocess addons/weapon_rifle_blank_c/config.cpp  > iterator_v.txt

real    0m4.193s
user    0m4.136s
sys     0m0.050s
bash-5.0$ time ~/projects/armake2/target/debug/armake2 preprocess addons/weapon_rifle_blank_c/config.cpp  > master_branch.txt

real    0m4.175s
user    0m4.111s
sys     0m0.057s

The final loop could obviously be replaced with a fold if you're so inclined.

I'd appreciate someone who knows what they're doing looking over this - the dereferences everywhere look like an anti-pattern, for example. Also, help with naming things would be useful.

I'm happy to tidy the PR up before (if?) it is merged.

src/preprocess.rs Outdated Show resolved Hide resolved
It's less easy to read but it is more fun
@bovine3dom
Copy link
Author

Merged @synixebrett's little sanity changes (thanks!).

bash-5.0$ time ~/projects/armake2/target/release/armake2 preprocess addons/weapon_rifle_blank_c/config.cpp  > master

real    0m0.684s
user    0m0.619s
sys     0m0.064s
bash-5.0$ time ~/projects/armake2/target/release/armake2 preprocess addons/weapon_rifle_blank_c/config.cpp  > iterated

real    0m0.655s
user    0m0.617s
sys     0m0.037s

Switched to loop for reduction and tested on "release" build. Still no slowdown.

@jonpas jonpas added the enhancement New feature or request label Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants