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

exploding compile times post development branch merge #158

Open
michael-kenzel opened this issue Jun 13, 2024 · 4 comments
Open

exploding compile times post development branch merge #158

michael-kenzel opened this issue Jun 13, 2024 · 4 comments

Comments

@michael-kenzel
Copy link
Contributor

michael-kenzel commented Jun 13, 2024

The changes incorporated with 62311a4 result in a massive increase in compile time. Compared to 47f4b88, we see compilation time for a random anyq benchmark rise from less than 1.5 seconds to over 7 seconds. The entire build slows down by over 27×, which makes this effectively a blocking issue, as what used to take < 3 min would now take well over an hour.

Based on some initial perf data (see below), it seems that artic post development branch merge spends almost half of the entire build time just rehashing hash tables during rewriting. THORIN_PROFILE does not yield any warnings regarding collisions, so this appears to be a consequence of the changes rather than just some issue with the hash tables.

repro.art, compile with --emit-llvm to reproduce problem

new (t = 7.2 s)

new

old (t = 1.4 s)

old

@Hugobros3
Copy link
Contributor

There appears to be a number of moderate perf regressions compounding to larger inefficiencies.

I pushed a couple commits today:

  • 0b7694d addresses poor performance due to pre-sizing very large hashmaps whenever a Rewriter is built. This happens constantly during PE because we create a rewriter to substitute arguments in a filter, whenever we evaluate a filter.
  • 1d5aa13 Gates some checks in scope.cpp to only be applied in Debug builds.

Let me know how much your perf improves, on my side I got the following numbers:

Commit time artic repro.art -O3 (Debug) time artic repro.art -O3 (Release)
1f938b7 (pre-merge) 3.48 0.48
62311a4 (post-merge) 25.81 5.14
1d5aa13 (current) 14.96 1.17

I'll update this post if/as I find other low-hanging fruit to optimize.

@PearCoding
Copy link
Contributor

PearCoding commented Jun 19, 2024

I did a quick and dirty benchmark as well on Ignis:

Commit Time
1f938b7 (pre-merge)* 8.877s
2400342 (post-merge) 44.512s
1d5aa13 (current) 30.209s

Note: The benchmarks are single-threaded, release build, run on a clean cache and contain a set of 10 shaders (large artic files). I used the diamond_scene.json for this benchmark.
Overall, the fixes improve the performance for me and do not crash :D But the difference to pre-merge is huge!

* This required Ignis master branch, instead of the development branch. But the code-complexity did not change between these.

@michael-kenzel
Copy link
Contributor Author

michael-kenzel commented Jun 21, 2024

Awesome, thanks! It's still about 5× slower (25 s vs 5 s) on my minimal test build here than pre-merge, but that's already a lot better than 27× slower 😄👍

@michael-kenzel
Copy link
Contributor Author

michael-kenzel commented Jun 21, 2024

I tested it now on the whole anyq build:

commit time
pre-merge (47f4b88) 133.6 s
current (1d5aa13) 299.8 s

so over the entire build, it amounts to about 2.3× slower than pre-merge. That's something we can work with :)

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

No branches or pull requests

3 participants