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

parallelise the code-gen #162

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

Conversation

danieleades
Copy link
Contributor

@danieleades danieleades commented Mar 21, 2023

inspired by @GabrielDertoni's work in #161, this PR provides a (much more modest) performance boost by parallelising the code-gen using rayon

Screenshot 2023-03-21 080654

@danieleades danieleades marked this pull request as ready for review March 21, 2023 08:08
@danieleades
Copy link
Contributor Author

danieleades commented Mar 21, 2023

open questions-

  • does rayon provide a speed up on all targets, or only some?
  • is it worth feature-gating this?

@GabrielDertoni
Copy link
Contributor

I would like to see how this does after #161 gets merged. Maybe the 1.84s of rayon comp time will already make it not worth it.

Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

Just some unnecessary style change to approve this, everything else looks awesome.

Cargo.toml Outdated
"icarous",
"common",
]
"all-dialects" = ["ardupilotmega", "asluav", "autoquad", "matrixpilot", "minimal", "paparazzi", "python_array_test", "slugs", "standard", "test", "ualberta", "uavionix", "icarous", "common"]
Copy link
Member

Choose a reason for hiding this comment

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

do not follow up style changes with funcional changes as well, having such list in a huge single line is harder to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem. This was introduced as an artefact of using cargo edit to add the rayon dependency (which can change formatting), not an intentional change

@danieleades
Copy link
Contributor Author

I would like to see how this does after #161 gets merged. Maybe the 1.84s of rayon comp time will already make it not worth it.

I agree it's worth merging your changes first and then reassessing.

I think also it maybe be worth considering initial compilation and subsequent compilation separately. For example maybe its worth compiling rayon once to get a benefit on every subsequent recompilation of this library

@danieleades danieleades marked this pull request as draft March 21, 2023 15:44
@danieleades
Copy link
Contributor Author

danieleades commented Mar 21, 2023

@GabrielDertoni on my machine, rayon costs 2.79s, in order save 1.2seconds to run the mavlink build script. On that basis it's not worth it for downstream users of this crate. It would be beneficial for developers on this crate who are recompiling repeatedly, but you're talking a 1 second improvement on a total compile time of around 60seconds, so it's a marginal gain.

I suspect the change isn't worth it on that basis, unless there's a more lightweight way to parallelise this without rayon.

Screenshot 2023-03-21 160357

@GabrielDertoni
Copy link
Contributor

Yes, well maybe there are some other ways to more significantly improve compilation speed. In particular in my measurements removing the serde reduces mavlink compilation time to ~1/3. If you additionally only have the common feature enabled instead of ardupilotmega it gets to ~1/6 the time. So, although it would be a breaking change, it would be good to eventually remove these from the default featues since they aren't even required for the tests to run.

@GabrielDertoni
Copy link
Contributor

GabrielDertoni commented Mar 21, 2023

Another way to significantly improve developer experience for this crate is using a separate crate for codegen, and letting it run build/parse.rs and including the generated code. This way, developers that don't want to change the parser, don't need to wait for the build script to rerun and all the exact same generated code to get compiled once again.

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

3 participants