c-cube / qcheck

QuickCheck inspired property-based testing for OCaml.
https://c-cube.github.io/qcheck/
BSD 2-Clause "Simplified" License
340 stars 37 forks source link

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

Open jmid opened 5 days ago

jmid commented 5 days ago

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

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:

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 commented 5 days ago

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

jmid commented 5 days ago

Rebased on main after merging #289