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

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

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

krame505
Copy link
Contributor

@krame505 krame505 commented Feb 5, 2024

See #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 B-Lang-org/bsc-contrib#25.

@quark17
Copy link
Collaborator

quark17 commented Feb 6, 2024

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
Copy link
Collaborator

quark17 commented Feb 6, 2024

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

@krame505
Copy link
Contributor Author

krame505 commented Feb 6, 2024

Sure, I can add that test case.

@krame505
Copy link
Contributor Author

krame505 commented Feb 6, 2024

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

@quark17 quark17 linked an issue Feb 6, 2024 that may be closed by this pull request
@krame505
Copy link
Contributor Author

krame505 commented Feb 6, 2024

Since B-Lang-org/bsc-contrib#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 quark17 merged commit 2b83a78 into B-Lang-org:main Feb 6, 2024
36 of 43 checks passed
@krame505 krame505 deleted the generics-either-tree branch February 6, 2024 21:26
@quark17
Copy link
Collaborator

quark17 commented Feb 6, 2024

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.

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.

BSC is slower after merging Generics feature
2 participants