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

Remove QCheck2.TestResult.get_instances to address memory leak #288

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

jmid
Copy link
Collaborator

@jmid jmid commented Jun 28, 2024

QCheck.TestResult.t.instances was originally added in dc53caf to address #51.
With the addition of QCheck2 it was moved there, QCheck.TestResult.t was made abstract, and instead

  val QCheck2.TestResult.get_instances : 'a QCheck2.TestResult.t -> 'a list

was exposed in the 0.18 interface.

Now https://sherlocode.com/?q=get_instances reveals that noone uses the binding, however #287 reveals that keeping all test inputs causes memory leaks

  • that increase with the test count
  • that accumulate further as a test list grows.

This PR therefore proposes to remove the list of instances from TestResult.

With the PR, on the test case from @esope QCheck2 (and QCheck) regains constant memory usage across increasing test counts - a nice property IMO (beware slight variation below due to different seeds):

$ /usr/bin/time --format="max memory: %M kB" _build/default/test_esope.exe 10000
random seed: 404829644
================================================================================
success (ran 1 tests)
max memory: 12628 kB
$ /usr/bin/time --format="max memory: %M kB" _build/default/test_esope.exe 20000
random seed: 234774561
================================================================================
success (ran 1 tests)
max memory: 12500 kB
$ /usr/bin/time --format="max memory: %M kB" _build/default/test_esope.exe 30000
random seed: 263321469
================================================================================
success (ran 1 tests)
max memory: 12628 kB

In addition, 12.6MB also seems more reasonable compared to the 300MB, 583MB, 881MB the above use to take!
I suspect performance will improve a bit, due to the decreased GC stress.

For the 2 cases reported by @Robotechnic the situation improves nicely too:

$ /usr/bin/time --format="max memory: %M kB" _build/default/robotechnic_list.exe
random seed: 243559907
================================================================================
success (ran 3 tests)
max memory: 64120 kB
$ /usr/bin/time --format="max memory: %M kB" _build/default/robotechnic_sep.exe
random seed: 490585175
================================================================================
success (ran 1 tests)
================================================================================
success (ran 1 tests)
================================================================================
success (ran 1 tests)
max memory: 64928 kB

To recap, in a list of 3 tests this uses ~64MB down from ~1151MB
whereas 3 separate runs uses ~65 MB down from ~438MB (again ignoring different seeds)

Because of the way we process tests:

  • run and print pass/fail as they execute
  • secondly print counterexamples and statistics afterwards

we cannot realistically obtain constant memory usage as test lists grow.

However, as the above numbers show, this seems much more acceptable in the light of the overall memory reduction this PR offers.


Edit - Addition:
I believe this will make an even bigger difference for QCheck2 test than for QCheck ones, as the former's generator produces larger lazy shrink trees.

@jmid jmid mentioned this pull request Jun 28, 2024
@jmid
Copy link
Collaborator Author

jmid commented Jun 28, 2024

Note: the CI failure is unrelated and should be addressed by #289

@jmid jmid force-pushed the remove-get-instances branch from a6aa075 to 55b3f85 Compare June 28, 2024 11:43
@jmid
Copy link
Collaborator Author

jmid commented Jun 28, 2024

Rebased on main after merging #289

jmid added a commit to ocaml-multicore/multicoretests that referenced this pull request Jun 28, 2024
@jmid jmid merged commit 2ca480c into c-cube:main Jul 4, 2024
11 checks passed
@jmid jmid deleted the remove-get-instances branch July 4, 2024 13:40
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.

1 participant