Turn tests/c-api/alloc_async.ml back on#11596
Conversation
|
What is the benefit? That is, what is this PR testing, and why does it matter? What is the risk? That is, are we 100% sure this PR will never fail randomly because of scheduling oddities, legitimate differences between operating systems, etc ? |
|
I don't follow either, but I'd still like answers to my two questions: what is being tested? is the test reliable? |
Paging @stedolan, who wrote the original test. Would you be able to tell us what's going on here? @gadmm most recently worked on improving when and where asynchronous callbacks may be triggered in OCaml 5, who may also be able to tell us whether this test is expected to be reliable in OCaml 5. |
testsuite/tests/c-api/alloc_async.ml
Outdated
| (* TEST | ||
| modules = "alloc_async_stubs.c" | ||
| * skip | ||
| reason = "alloc async changes: https://github.com/ocaml/ocaml/pull/8897" |
There was a problem hiding this comment.
The reason line seems to explain why the test is there, rather than why it had been disabled. Let's keep it.
There was a problem hiding this comment.
reason sets the message for the outcome of the test:
ocaml/ocamltest/builtin_variables.ml
Lines 65 to 66 in 17d9f7c
It could be moved to a comment?
There was a problem hiding this comment.
Ah, I imagine this was added as a reference while rebasing multicore (meaning "we must reimplement 8897 at some point"). This is now done, so it is a reason to put it back in. Instead, a comment would be welcome to explain that this is a test that allocations from C code do not run finalisers.
There was a problem hiding this comment.
Description added (see below)
|
This appears to test that the asynchronous callbacks do not run inside C code even when doing all sorts of allocations from C code. I think this is a nice test to have even though it relies on some implementation-dependent details, because if the behaviour changes at all it better be intentional. The C stub tries to complete a major collection to make sure the finaliser enters the queue. I do not know if its logic is still valid (I have in mind full_major which now requires 3 cycles instead of 2, and I am not familiar with these sorts of changes to the GC behaviour). Note that you cannot call full_major because it would call finalisers immediately.
This test should not depend on scheduling oddities (given that it is single-domain).
Only the legitimate differences between native and bytecode come to mind, but the test should work for both. |
6536816 to
bede269
Compare
gadmm
left a comment
There was a problem hiding this comment.
Assuming the trick to complete a full major collection still works, this looks good to me.
|
From (hopefully correct) poking, the allocation loop does indeed cause finalisers to be enqueued and running the test with v=0x80 shows that those finalisers do not run until the C code has returned (as required). |
|
Am I OK to merge this? |
|
ping @gasche or @kayceesrk |
|
I need this test, and found that it could be made more robust (without changing the fact that it currently passes). Essentially, the allocation at the end is meant to trigger async callbacks, but instead, the polling is already done via To fix this, I propose to replace Regardless of this improvement, I'd like to see this PR merged as it is useful for the async callback mechanism. |
|
I haven't looked at this, but I am happy to approve on @gadmm's behalf if his last proposal is addressed. |
|
The testcase has finally been restored with the improvements I proposed as part of #11307. This PR can therefore be closed. |
Follow-up from #10861, this test appears to be working (famous last words...)