Normative: Describe processing model and refine layering

[littledan]

No description provided.

[littledan]

Companion PR: whatwg/html#4571 . cc @domenic

[erights]

Why "[[Target]] field or internal slot" ? If the [[KeptAlive]] slot is treated as the [[Target]], doesn't that mean it fails to keep the object alive until end of job (aka turn)?

[littledan]

Note, this text is not new to this PR; I just moved it around (and rephrased the reference to KeptAlive, but not the part you're referring to). I'll think about how to clarify it further, but I think we could do this in a separate patch.

[erights]

What is "empty" ?

[littledan]

It's a sentinel used in this specification inside WeakRefs when the GC nulls out a pointer.

[erights]

It seems like this text is going out of its way to avoid just calling these Jobs (aka turns), and leaving it to the host to determine when these Jobs interleave with other Jobs. The language seems to lose the clarity of Jobs as a unit of atomicity.

[littledan]

I don't think we have the machinery in ECMAScript to specify that these are scheduled the way that they should be, to not interrupt a higher priority sequence of Promise reactions.

We could specify this in terms of Jobs, but this would force HTML to return to making another "willful violation" in order to specify how they are ordered with respect to other Jobs (since the HTML event loop isn't in terms of ECMAScript Jobs).

We know from experience that such "willful violations" are confusing to implementers. In particular, I'm concerned that someone could see this Job language and conclude that these actions are interspersed with Promise reactions.

[erights]

The text that you wrote already allows these to be interleaved with promise jobs.

The html spec not being written in terms of ES jobs seems like a bug. Why is any willful violation necessary?

[littledan]

That's true, it does allow it to be interleaved with Promise Jobs, but it also allows embedder specifications to precisely say that they are not interleaved. If we said it in terms of Jobs, then it's really difficult to specify the ordering--RunJobs says that the ordering is implementation-defined (not embedder-defined) and is generally hard to plug into while providing very little actual semantics for itself.

I wrote a presentation explaining the willful violation and how we can avoid it: slides. I was going to present this at the March 2019 TC39 meeting, but delayed it as there's a bit of ongoing refactoring in the event loop at the HTML level. At this point, I'm thinking of presenting basically the same thing in June.

If it's really important to you, I can rewrite this in terms of ES Jobs, but then I'll be forced to broaden the willful violation in HTML. That doesn't make me happy.

[erights]

implementation-defined (not embedder-defined)

Interesting distinction. I assumed that hosts could define anything that was implementation defined.

At this point, I'm thinking of presenting basically the same thing in June.

Please do!

If it's really important to you, I can rewrite this in terms of ES Jobs, but then I'll be forced to broaden the willful violation in HTML. That doesn't make me happy.

Please don't. As long as this text is coherent and correct to both, ok. But I'll be really glad to see your presentation in June, because we need to heal this semantic mismatch between the specs, without making the JS spec any less appropriate for non-browser hosts. Once healed, hopefully we won't be faced with such bad choices in the future.

[littledan]

I share the goal of making the JS spec appropriate for non-browser hosts. I'd like to consider other possibilities for making the specification precise for these various scenarios. I'm glad we agree that it's important that we fix the mismatch.

I think, at this point, we can agree that it's an (important!) editorial question we're talking about, and not a normative one. Can we work towards resolving this question during Stage 3/while reviewing that presentation, unblocking WeakRefs from advancing?

The big semantic point that this patch tries to make, together with the HTML PR, is to say, "[to the extent that we can say this] WeakRef deref() holds the object live until until after all the Promise reactions run, and the FinalizationGroup callbacks are not interspersed in Promise reactions; the garbage collector may launch FinalizationGroup callbacks even if JS is idle".

The parenthetical at the beginning is because JS currently leaves so much up to hosts, so it's hard to say anything about Promise reactions happening before anything else in particular. However, hosts have so much going on in them, that it's hard to figure out how to say much more at the JS level. Maybe we should have a declarative statement somewhere explaining the prioritization, to be specified imperatively in more detail by the host.

[mhofman]

WeakRef deref() holds the object live until until after all the Promise reactions run

What is the reasoning behind this requirement? I thought the goal was to prevent collection of those targets during the current synchronous code.
The note for KeepDuringJob says:

This may be abstractly implemented as a Set in the current Job that becomes unreachable when the Job ends because the Job becomes unreachable, or as a Set in the current Agent that gets cleared at the end of each Job.

Wouldn't that suggestion allow collection before the Promise jobs queue is emptied?

let wr = new WeakRef({});
Promise.resolve().then(() => {
  gc();
  console.log(wr.deref()); // is undefined allowed here?
});

[littledan]

No, undefined is not allowed there. The "Job" terminology is ambiguous, so this PR switches to using host hooks that define things more precisely. The kept set is not cleared except by a new task, which will run only after a microtask checkpoint.

[erights]

Hi @littledan thanks for your careful attention to this. Given the current state of the world, this seems like a fine basis for proceeding.

[littledan]

@erights Thanks for your prompt and careful review!

[littledan]

Landing this PR to make reviews easier. We can continue to iterate on it later.

[mhofman]

I just realized a side-effect of this PR. Before, calling finalizationGroup.cleanupSome() would always invoke callback even when no cells were empty. The iterator would simply not yield anything.
After the change, the callback is not invoked if the finalization group doesn't have any empty cell.

Just wanted to double check the new behavior was intended.

[mhofman]

This might make some implementations harder: now the finalization group either needs to check all cells immediately when cleanupSome is called instead of relying on the iterator use by the caller, or it need to internally keep track if it has an empty cell.

[littledan]

Yes, this line of this PR does make that change. Should we remove it?

[littledan]

I don't think this is such a big burden, since something will have to be found when looking for the .next() entry anyway

[mhofman]

It make some of my tests more complicated ;)
So far I was relying on calling cleanupSome to get access to the iterator. Now I can't do that anymore without forcing a gc.

I just rewrote my shim to include that new behavior, and fix the scheduling of "host" jobs.
Keeping track of empty cells allowed me to avoid iterating blindly over all cells in the cleanup job. Not sure how real engines would implement this, but in my case I wouldn't consider it a big burden.

[littledan]

OK. I think we should leave the semantics as is for now, but if we hear about more problems, we can consider reverting it.

[mhofman]

Looks like the operation is named CleanupFinalizationGroup in the rest of the document.

[littledan]

Ah, right. Interested in making a PR to correct it?