Skip to content

Upgrade Polysemy to latest to fix OOM in Galley.Run#2947

Merged
mdimjasevic merged 7 commits intowireapp:developfrom
isovector:fix-oom
Dec 22, 2022
Merged

Upgrade Polysemy to latest to fix OOM in Galley.Run#2947
mdimjasevic merged 7 commits intowireapp:developfrom
isovector:fix-oom

Conversation

@isovector
Copy link
Contributor

@isovector isovector commented Dec 20, 2022

This PR applies a fix in Polysemy to dramatically cut down on (useless) compile-time inlining.

Currently marked as draft to see if it does in fact fix CI. If so, I'll release a new version of polysemy directly rather than depend on a pinned commit.

Update

@mdimjasevic and @pcapriotti ran a local memory usage comparison. This PR does seem to help substantially: before the patch compiling galley was using 49584 MB of RAM on @pcapriotti's system, compared to 34392 MB after the patch. It's still insane, but it's an improvement of about 44%.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@isovector isovector temporarily deployed to cachix December 20, 2022 20:08 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 20, 2022 20:08 — with GitHub Actions Inactive
@isovector
Copy link
Contributor Author

@smatting

@julialongtin julialongtin added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Dec 20, 2022
@isovector isovector temporarily deployed to cachix December 20, 2022 23:46 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 20, 2022 23:46 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 21, 2022 00:01 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 21, 2022 00:01 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 21, 2022 01:51 — with GitHub Actions Inactive
@isovector isovector temporarily deployed to cachix December 21, 2022 01:51 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 21, 2022 12:07 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 21, 2022 12:07 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic marked this pull request as ready for review December 21, 2022 12:07
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Thanks for this!

It would be great if you could make another PR once you release a new Polysemy version so we can drop the pins from nix/haskell-pins.nix.

@mdimjasevic
Copy link
Contributor

@isovector , do you have any observations when it comes to improvements in compilation performance that this PR brings?

@isovector
Copy link
Contributor Author

I don't; but this patch lets me build galley on my laptop with 16GB of RAM. Polysemy really doesn't do much that should trip up the optimizer with the exception of trying to inline all of its Member proofs, so this patch eliminates its desire to do that.

@mdimjasevic
Copy link
Contributor

I don't; but this patch lets me build galley on my laptop with 16GB of RAM. Polysemy really doesn't do much that should trip up the optimizer with the exception of trying to inline all of its Member proofs, so this patch eliminates its desire to do that.

Just to confirm: was this with the O2 optimization level? That's the most challenging way of compiling we've been facing.

@mdimjasevic mdimjasevic temporarily deployed to cachix December 22, 2022 10:12 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 22, 2022 10:12 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 22, 2022 10:56 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix December 22, 2022 10:56 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic merged commit 1a29aa0 into wireapp:develop Dec 22, 2022
@mdimjasevic
Copy link
Contributor

Thanks Sandy for the PR! It significantly reduces the hardware requirements for building Wire server.

Now if you could make a Polysemy release with this so we can unpin it in Nix, that would be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants