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

Memory corruption fixes #6697

Merged
merged 2 commits into from
Nov 13, 2020
Merged

Memory corruption fixes #6697

merged 2 commits into from
Nov 13, 2020

Conversation

mrmr1993
Copy link
Member

This PR

  • removes the optimised, no-double-copy code for converting Vec<A> to ocaml::Array<B> in rust
  • replaces the uses with an explicit map Vec<A> -> Vec<B> and falls back on the implicit Vec<B> -> ocaml::Array<B> conversion

This appears to fix a memory corruption issue, where a bad allocation for a polynomial commitment could cause a Fatal error: out of memory message. The tool I wrote to find the bug would consistently trigger this in < 5000 iterations on the old version; the new version still hasn't after 200,000 (test still running).

I'm not clear on why this fixes the issue: the underlying logic for the two versions is nearly identical, save for an extra (presumably unnecessary) vector allocation in the new version. However, in the old version the compiler seems to optimise away some writes that are needed to tell the OCaml GC about some live heap memory; in the new version these writes appear happen successfully.

Checklist:

  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them:

This fixes #6674

@mrmr1993 mrmr1993 added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label Nov 13, 2020
@mrmr1993 mrmr1993 requested a review from a team as a code owner November 13, 2020 03:57
@mergify mergify bot merged commit 1b339e0 into develop Nov 13, 2020
@mergify mergify bot deleted the feature/remove-rust-caml-vector branch November 13, 2020 23:19
@mrmr1993 mrmr1993 mentioned this pull request Nov 13, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch ready-to-merge-into-develop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatal error: out of memory
2 participants