(cc potential reviewers: @damiendoligez @bobot @jhjourdan @stedolan @kayceesrk)

Just to rephrase and complete to see if our understanding are in sync:

• It is an optimization for the clean phase (I'm surprised that we spend so many time in this phase, do we know why?)
• The clean phase is incremental, but we don't know which ephemerons have been cleaned (the dead values are removed)
• The goal of the clean phase is that even if all the dead values are not yet removed from the ephemeron we want to do as if.
• So each argument of blit could be uncleaned or already cleaned

So:

• if dst (ard) has already been cleaned we should not add unclean keys: but the only keys added are the one blitted and they are checked by the new partial clean in src (ars).
• if dst is unclean: we should not remove all the unclean keys without cleaning the data, the only keys removed in dst are checked in the new partial clean.

I agree with the idea of the optimization, and in fact it seems that we could avoid the partial clean of dst if the data of dst is caml_ephe_none (weak array case).

I agree on every point.

I don't know why the clean phase takes much time (or what fraction of time it's supposed to take, anyway). Maybe in this benchmark it's extreme because most (all?) of the objects allocated on the major heap are ephemerons, but we've seen a noticeable performance hit in a real application where I think <5% of the heap consists of ephemerons. Anyway, whatever the proportion is, if it's constant we ended up with a quadratic cost of blit elementwise blitting asymptotically.

it seems that we could avoid the partial clean of dst if the data of dst is caml_ephe_none

That sounds mostly right. I don't know whether it's safe/desirable to call do_set with an unclean destination, though. Do you think this is safe and worth doing?

Do you think this is safe and worth doing?

I think it is safe, because the only thing checked on the old key is if it is young. And a key can't be young and unclean.

Is it worth doing?

The patch is a oneliner, but I have no experimental comparison.

I pushed the optimization. It doesn't seem to affect the benchmark I have, presumably because the destination is always filled with nones and scanning those is very cheap (compared to caml_page_table_lookup).

Still, cleaning the source array constitutes just over 10% of the cost of Weak.blit (which itself, by the way, is only ~20% of the overall benchmark time now) so saving some cleaning could well be non-negligible in some cases.

Wait, actually I don't agree with your safety argument. A key that's unclean and being overwritten is in fact guaranteed to visit the if (!(Is_block (old) && Is_young (old))){ branch in do_set, and, therefore, add_to_ephe_ref_table, with consequences that are beyond my understanding.

Given that it seems tricky to make a safety argument and the win is not great, I weakly prefer if we undo the optimization.

What do you think? (or maybe I miscounted the number of negations :-D)

Oh, maybe the answer is that this branch is visited in both cases (because caml_ephe_none is not a young block either)?

The set is already done when add_to_ephe_ref_table (Caml_state->ephe_ref_table, ar, offset); is run, so old is not in the picture anymore (and in fact the key is not stored in the table). The value old is only given to Is_block and Is_young.

Yeah, I do agree now. Thank you for clarifying.

I adjusted the changelog and collapsed the commits into two: one that introduces caml_ephe_clean_partial and another that implements the optimization proposed by @bobot. Please let me know if I should do anything else before this PR can be merged.