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

Don't include json_loader.h in ir.h #643

Merged

Conversation

sethfowler
Copy link
Contributor

Here's a PR I've summoned from an old branch.

p4c takes a long time to compile, and one of the biggest factors is duplicate template instantiations in different translation units. We've worked around this to some degree with unified builds, but it'd be much better to resolve the underlying problem.

This patch takes one small step in that direction by eliminating the need to include json_loader.h in ir.h. This is accomplished by adding forward declarations for JSONLoader where needed (including in ir-generated.cpp) and moving the code that references JSONLoader from ir-inline.h into json_loader.h itself.

I had tried to do the same for json_generator.h, but while I have a patch that does it, it requires some icky explicit template instantiations. Getting rid of it cleanly would require a redesign that didn't implement toJSON() as a virtual method.

@sethfowler sethfowler requested a review from ChrisDodd May 18, 2017 00:41
@sethfowler sethfowler self-assigned this May 18, 2017
Copy link
Contributor

@ChrisDodd ChrisDodd left a comment

Choose a reason for hiding this comment

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

Does this really make much speed difference? It only removes the templates from compilation units that never instantiate them, so the only savings is the time to read and parse one file...

@sethfowler
Copy link
Contributor Author

No, it doesn't make much speed difference. It's just a baby step towards the larger goal of minimizing the amount of code that gets #include'd in each file.

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