search

B-Lang-org / bsc

Bluespec Compiler (BSC)
Other
955 stars 146 forks source link

Change derived Generic sum type instances to generate a balanced binary tree of Eithers #670

Closed krame505 closed 9 months ago

krame505 commented 9 months ago

See https://github.com/B-Lang-org/bsc/issues/334.

This makes the derived instances for Generic for datatypes with multiple constructors use a balanced binary tree of Either types, rather than a linear chain of Eithers. This has the side-effect of making it much nicer to write generic Bits-like type classes, see the changes to CustomBits.bs in the testsuite.

To measure the effect on performance, I ran time make checkparallel -j16 -C testsuite before and after making this change. Pre-change (commit e4361d9) testsuite runtime: 10383.02s user 1748.27s system 666% cpu 30:20.54 total Post-change (commit d32b927) testsuite runtime: 7877.59s user 1738.98s system 1325% cpu 12:05.69 total

Note that when running the testsuite prior to this change, there were some tests that failed due to OOMing on my system, so the exact speedup isn't accurate - but at least this is clearly a significant performance win. Is there a more specific test/set of tests that exhibit this problem, that would be a better benchmark?

Note that the bsc-contrib tests are currently failing, but should be fixed by merging https://github.com/B-Lang-org/bsc-contrib/pull/25.

quark17 commented 9 months ago

This is awesome! Thank you for looking into it!

Is there a more specific test/set of tests that exhibit this problem, that would be a better benchmark?

Discussion #414 has an attached file enums.zip (containing IPv4.bsv). With the current HEAD, running bsc -v IPv4.bsv takes a long time on my old macOS, probably partly because it hits the 16GB physical memory limit -- I see 43s in typecheck, 138s in internal, and it is currently churning just after the internal stage, probably in the sanity typecheck that is done after most stages and isn't included in the accounting for each stage. With your changes, it runs in 9 seconds (2s in typecheck and 2s in internal)!

It might make sense to include that example in the testsuite now. Perhaps a new directory bsc.bugs/github/gh334/? Do you want to add that, or I can do it?

quark17 commented 9 months ago

Also, I like how this greatly simplifies the source code in the CustomBits example! Thank you again for this!

krame505 commented 9 months ago

Sure, I can add that test case.

krame505 commented 9 months ago

Done. I think we can safely say this closes #334?

krame505 commented 9 months ago

Since https://github.com/B-Lang-org/bsc-contrib/pull/25 is merged this should be passing now, but I don't see a way to re-trigger the github checks. Or maybe a maintainer needs to do that?

quark17 commented 9 months ago

Ah, I probably should have re-run the failing jobs. I ran the tests locally and confirmed that they passed, so I decided to merge.

The view of the checks on the PR page is a bit confusing, so I usually click through to the Actions tab and view the checks there. That full view offers a button in the upper right to re-run failed jobs, but I don't know if that's available to the author of the PR or just to me as maintainer.