Simplify lazy semantics

[kayceesrk]

Simplify lazy and fix a bug based on discussions from #749 and #750.

• Mark lazy as not concurrency (domains, systhreads, fibers) safe.
• Remove RacyLazy exception. Any concurrent use of lazy value may raise Undefined exception.
• Remove domain-local id to distinguish concurrent and recursive forcing.
• Remove try_force.

The commit also fixes a bug to prevent short-circuiting forcing_tag values.

We need to bootstrap to remove references to RacyLazy from the bootstrap binaries. After bootstrap, the primitive
caml_ml_domain_unique_token should be removed from the runtime.

[kayceesrk]

@gasche would appreciate your review.

[gasche]

Thanks for the ping! (I don't get notifications for ocaml-multicore/)

The change looks correct to me. It does simplify the implementation.

The documentation is changed so that Lazy.force is "unspecified" (but safe) in the case of concurrent forcing. I think we could be more precise, and will try to make some proposals.

The PR comes with some changes in the testsuite, some for backward-compatibility, some (dividing iteration counts by 10 or removing some cpu_relax() call) that seem to come from performance considerations, I'm not sure which/why.

[gasche]

... after thinking more about it, I think that the current non-documentation of concurrent-forcing behavior is okay.

[gadmm]

I have more questions about this code now that I understand it better. Some comments are not about behaviour introduced by this PR, feel free to address them later/separately.

[kayceesrk]

Also, thanks @gasche and @gadmm for your high-quality reviews.

[kayceesrk]

The failures are due to line number differences in the backtrace tests. I've already fixed these twice in this PR :-| I'll wait until the review on the changes reaches a fixed point, and then I shall fix the failing tests.

[gasche]

My high-level impression is that we should try to merge a "good first part" version of this PR, that simplifies the current implementation and does not introduce any new bugs, and then have a separate, second discussion about how to safely read concurrently-modified tags. (I suspect we may not be out of surprises there: many other parts of the runtime read tags, how safe are they with respect to concurrent mutation?)

[gasche]

To continue on my previous line of thought: I think we could cut the current PR to the commit before "Add new primitive to atomically read tag", and get this merged right away, and then have a dedicated discussion on the memory-safety to make sure that we properly address the issue.

(@kayceesrk : just checking that you know about make promote DIR=... to auto-fix line numbers and in generate test outputs in the testsuite.)

[gadmm]

I think we're there right after changing lazy_..._switch to lazy_..._cond or to what @gasche proposed, and changing obj_tag to address @xavierleroy's concern. It's an easy fix so no need to delay the last part of this PR.

[gasche]

What about the lazy-tag checks in other parts of the runtime?

• compare.c, hash.c: I think they need an atomic read to handle Forward_tag safely
• extern.c: this logic can run concurrently with another mutator, but iirc. we decided that it is unspecified what happens in this case (but no crashes); I think the current code will not crash if it reads the tags wrong, at worst it will incorrectly shortcut a pointer in the serialized result. No change seems required.
• finalise.c: no change is required
• minor_gc.c, major_gc.c, weak.c: no change required as iirc. no mutators can run concurrently with the GC

[bluddy]

compare.c, hash.c: I think they need an atomic read to handle Forward_tag safely

Why? Worst case you get the wrong result (which is fine), but do you get a crash?

[gasche]

I think you are correct: we can get garbage in this case, but no crash, because the block arguments of a lazy value remains well-formed at any point. (With the new policy, specified in the current PR, that lazy values are not thread-safe, this is fine. Previously we tried to make them shareable, as long as only there is no racy force.)

[kayceesrk]

Based on the recommendations, I have

1. Removed the new primitive to atomically read tag and made obj_tag load the tag using acquire.
2. Incorporated @gasche's suggestion for inline_lazy_force_switch

just checking that you know about make promote DIR=... to auto-fix line numbers and in generate test outputs in the testsuite.

I did not know this. Thanks for pointing this out.

[gadmm]

LGTM. I would just add a comment (below).

[gasche]

(Note: my inline_lazy_switch suggestion requires massaging from the matching+backend people; if we merge as-is, as I think is reasonable, I will open an issue to track this.)

[gadmm]

(Note: my inline_lazy_switch suggestion requires massaging from the matching+backend people; if we merge as-is, as I think is reasonable, I will open an issue to track this.)

Or that could be used as an incentive to explore an implementation that does not mutate tags!

[kayceesrk]

Will merge when the CI turns green. Thanks @gasche, @gadmm and @xavierleroy.