Lwt_preemptive: work around Event.sync perf issue.

[mfp]

Refer to

http://caml.inria.fr/mantis/view.php?id=7158
#218.
mfp/ocaml-sqlexpr#13

Fix #218.

[mfp]

Extra eyeballs appreciated, it's very late over here :-/
FWIW "the one" test passes :)

[aantron]

@mfp: Do you have some idea, from your discussions and experience, what the impact of resolving the two Mantis issues (1, 2) will be? As in, is there a nice way to resolve them, such that Event creating all those Condition.ts won't be a performance issue anymore?

If performance will likely still be a problem, we should definitely merge this. If it won't be a problem anymore, we can still merge this, but I'm only about 75% convinced in that case – because it's a vanishing problem given the target of 4.03.1, and because it adds code. In that case, further arguments are welcome :)

[aantron]

cc @hcarty since he is working on a potential replacement or alternative to Lwt_preemptive.

@hcarty: would be interested about your experience/opinion on Mwt or Lwt_preemptive being subjected to the kind of usage that causes problems here.

[hcarty]

I haven't run into this largely because the cases where I've used Lwt_preemptive have been for heavier calls. So some extra GC overhead would be lost in the noise.

That is likely to change soon though so I would be happy to see performance improvements here! My main question is if this should live in Lwt or as a separate module/library in opam, used by Lwt.

[mfp]

On Tue, Jun 21, 2016 at 04:59:08PM -0700, Anton wrote:

@mfp: Do you have some idea, from your discussions and experience, what the impact of resolving the two Mantis issues (1, 2) will be? As in, is there a nice way to resolve them, such that Event creating all those Condition.ts won't be a performance issue anymore?

If performance will still be a problem, we should definitely merge this. If it won't be a problem anymore, we can still merge this, but I'm only about 75% convinced in that case – because it's a vanishing problem given the target of 4.03.1, and because it adds code. In that case, further arguments are welcome :)

The moment these issues are resolved, this PR will become unnecessary.
However, I'm not sure MPR7158 can be readily solved in a direct way (TBH, I
haven't made any serious attempt to understand that code, which is at any rate
much trickier than the present PR), and addressing MPR7198 would require work
on the GC and/or a new API, which could take a long time to happen (regardless
of the target version... I'm not sure it means it will positively be fixed
by that time).

Note that this PR addresses the performance issue as far as detach is
concerned, but run_in_main will still be affected (this could be solved
easily by pooling the mutexes/conditions or cells, at the cost of some more
code).

I'd say there are two arguments for merging this, and one and a half against:

pros:

• merging this solves the detach issue now
• and for all OCaml versions

cons:

• I might have introduced a bug (the code looks easy to inspect to me, though)
• this adds code, but given Lwt_preemptive's history (no significant changes
  in the last 4 years), it would be easy to revert at a later point in time if
  the MPRs are addressed directly and older OCaml targets become irrelevant

Mauricio Fernández

[mfp]

On Fri, Jul 01, 2016 at 06:39:53PM -0700, Hezekiah M. Carty wrote:

I haven't run into this largely because the cases where I've used Lwt_preemptive have been for heavier calls. So some extra GC overhead would be lost in the noise.

That is likely to change soon though so I would be happy to see performance improvements here! My main question is if this should live in Lwt or as a separate module/library in opam, used by Lwt.

The fix is specific to Lwt_preemptive's use of Event -- it does not solve the
overall Event/GC issue. The "cell" implementation is small enough to live
comfortably in Lwt_preemptive and avoid both the extra work to release it as a
separate OPAM package and adding a new dependency to Lwt.

Mauricio Fernández

[aantron]

Ok, I am convinced :)

My main question is if this should live in Lwt or as a separate module/library in opam, used by Lwt.

Assuming you mean module CELL, I don't think it's worth it at this point – this is just a regular data structure + CV + mutex. Plus, I don't think stdlib threads are used that much.

The change is not so trivial actually, because it replaces a queue (channel) with a single cell (the mutable option), but it appears to be correct.

Rebased on master, committed.

but run_in_main will still be affected

I'm adding a comment, in a follow-on commit, about the potential performance issue, above the implementation of run_in_main, so that if someone is ever debugging that, they might be able to save some time by quickly finding this PR and #218.

Thank you!