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

Use NoThunks to verify no memory leaks in PeerMetrics #4633

Merged
merged 6 commits into from
Aug 4, 2023

Conversation

coot
Copy link
Contributor

@coot coot commented Jul 27, 2023

  • ouroboros-network-framework: added NoThunk orphaned instances
  • peer-metrics: use NoThunks in PeerMetrics

@coot coot requested review from newhoggy, a team and bolt12 as code owners July 27, 2023 17:43
@coot coot removed request for a team and newhoggy July 27, 2023 17:43
@coot coot force-pushed the coot/peer-metrics-nothunks branch from 317ee05 to 3eed982 Compare July 27, 2023 17:46
@coot coot force-pushed the coot/peer-metrics-nothunks branch from 3eed982 to 5979d62 Compare July 28, 2023 20:05
@coot
Copy link
Contributor Author

coot commented Jul 28, 2023

Some test were failing because we also need to evaluate arguments of newPeerMetrics' to normal form. I added a bunch of NFData & NoThunk instances.

We should add NFData instance to IP in the iproute package (and NoThunk instance too).

@coot coot force-pushed the coot/peer-metrics-nothunks branch 6 times, most recently from 3d29c1d to 36a5aab Compare August 2, 2023 14:57
cabal.project Show resolved Hide resolved
nix/ouroboros-network.nix Outdated Show resolved Hide resolved
@coot coot force-pushed the coot/peer-metrics-nothunks branch 2 times, most recently from 487db33 to 503d7b3 Compare August 3, 2023 08:59
@coot coot force-pushed the coot/peer-metrics-nothunks branch 11 times, most recently from e7bd2c0 to 094e933 Compare August 4, 2023 08:14
@coot coot force-pushed the coot/peer-metrics-nothunks branch from 094e933 to 92a97f1 Compare August 4, 2023 08:28
Use NoThunks library to verify that we don't have any unevaluated
expressions in PeerMetric.

This patch also makes PeerMetric more strict to satisfy the test.
Apparently there was a small memory leak which goes away at every churn
interval when we actually use peer metric in order to demote the 20% of
worse performing peers.
@coot coot force-pushed the coot/peer-metrics-nothunks branch 2 times, most recently from 1f74f81 to 2bff827 Compare August 4, 2023 11:16
Also removed `asserts` flag from `network-mux.cabal`; and updated
`cabal.project.local` files used in GHA.
The `continueWithState` and `continuewWithStateM` functions need
`NOINLINE` pragma to make sure the `st` argument is evaluated to WHNF.
@coot coot force-pushed the coot/peer-metrics-nothunks branch from 2bff827 to f2061c9 Compare August 4, 2023 14:39
@coot coot enabled auto-merge August 4, 2023 14:40
@coot coot added this pull request to the merge queue Aug 4, 2023
Merged via the queue into master with commit a0b1a91 Aug 4, 2023
9 of 110 checks passed
@coot coot deleted the coot/peer-metrics-nothunks branch August 4, 2023 20:22
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.

None yet

2 participants