Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggested changes to PR #1597 #4

Open
wants to merge 8 commits into
base: enqueue-promise-job
Choose a base branch
from

Conversation

jmdyck
Copy link

@jmdyck jmdyck commented Sep 11, 2019

suggested changes to PR tc39#1597

domenic and others added 8 commits September 2, 2019 16:19
This implements the proposal discussed in tc39#240 for clarifying the requirements of the job infrastructure while removing the overly-constrictive framework provided by the previous definitions for EnqueueJob, and the unused-by-hosts RunJobs, TopLevelModuleEvaluationJob, and ScriptEvaluationJob. Previously, no real-world host was able to integrate easily with the existing infrastructure, without taking excessive advantage of the implementation-defined steps in order to force-feed the job queue one job at a time, and never using the "ScriptJobs" queue. In this way, the actual requirements were obscured by the ceremony around implementing them.

Now, the requirements that the job framework is meant to capture are listed explicitly in the definition of the new HostEnqueueJob abstract operation, which hosts are allowed to implement in a way that integrates more easily with their own frameworks (see e.g. https://html.spec.whatwg.org/#enqueuejob(queuename,-job,-arguments)). The algorithms in RunJobs, TopLevelModuleEvaluationJob, and ScriptEvaluationJob have been moved to an informative annex giving an example command-line ECMAScript implementation; they remain a good illustration of how a simple host could work, despite being unusable by real-world hosts. Similarly, the PendingJob framework and the algorithm previously in place in EnqueueJob have been moved to this annex as a way of illustrating a possible HostEnqueueJob implementation. The annex is then fleshed out by also defining possible algorithms for the remaining host-defined abstract operations (HostEnsureCanCompileStrings, HostResolveImportedModule, and HostPromiseRejectionTracker) in such a command-line implementation.
Rather than using a string queue name, this patch uses separate hooks
for separate things that may be scheduled, in order to permit the
clear expression of invariants which differ based on the particular
type of job. For example, Promise Jobs are required to be served in
FIFO order, whereas WeakRef Jobs are unordered.
In tc39#1597 (comment)
@domenic points out that:
> The stack-pushing is just a kind of weird way of getting
> a return value out of InitializeHostDefinedRealm().

This PR should simplify
https://html.spec.whatwg.org/multipage/webappapis.html#creating-a-new-javascript-realm

It's also pretty much necessary for the next commit.
... mainly to bring agents into it more.

(Also delete 2 'es6num' lines.)

Note that the sentence
"Only one Job may be actively undergoing evaluation at any point in time."
is redundant given the run-to-completion requirement.

(In fact, even the run-to-completion sentence is pretty much redundant
given the requirement to start when the execution context stack is empty.
But I think it's useful.)
It's simpler and I think it reads better.

The major benefit would be when you can add a new "job group"
with as little as one line.

I called it 'jobGroup' (rather than the analogous 'queueName' parameter
of EnqueueJob in the status quo spec) because I don't think
the spec can dictate the queues that the agent must have.

Note that HostEnqueueJob's parent clause is basically redundant:
everything it says is also conveyed by the HostEnqueueJob clause.
But I left it there in case it becomes more of a useful overview.
Copy link
Owner

@littledan littledan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'm not a big fan of this editorial change. I'll accept it if others see it as necessary to land the broader patch, as it preserves the important layering qualities, but I think it adds unnecessary confusion.

@@ -6614,6 +6616,41 @@ <h1>Agents</h1>
<p>An agent is a specification mechanism and need not correspond to any particular artefact of an ECMAScript implementation.</p>
</emu-note>

<emu-clause id="sec-agent-behaviour">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this section confusing, and I'm not sure what it adds. I'm not sure what's missing from the descriptions in those algorithms to understand that you can call them this way.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this section confusing,

I'd like it to be less confusing. I'm not sure it can be. Do you have any suggestions?

and I'm not sure what it adds. I'm not sure what's missing from the descriptions in those algorithms to understand that you can call them this way.

It's not so much that you can call them this way but that you must call them this way. Or at least, you must if you want the ES implementation to do anything sensible.

The spec doesn't spend much effort to say how you can call an operation. For the most part, that's okay, because what matters more (at least from the reader's point of view) is how it is called, which they can see elsewhere in the spec. But, in PR tc39#1597, these 6 operations become top-level operations, and there's zero information about how they're invoked.

If you don't single out these 6 operations, then are they on the same footing as every other operation? Is the ES spec just a box full of thousands of parts, that the host can use however it wants?

<li>Once evaluation of a Job starts, it must run to completion before evaluation of any other Job starts.</li>
</ul>
<emu-clause id="sec-hostenqueuejob" aoid="HostEnqueueJob">
<h1>HostEnqueueJob ( _jobGroup_, _job_, _arguments_ )</h1>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to use separate callbacks, rather than passing this string "jobGroup" argument, since in callsites, the string is fixed, and in embedders, the different queues will be processed differently. This phrasing is a bit like serializing the name of the algorithm in a string.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to use separate callbacks, rather than passing this string "jobGroup" argument

I realize you have that preference, but I don't understand why, and thought that this commit might provide a basis for discussion.

since in callsites, the string is fixed,

Yup.

and in embedders, the different queues will be processed differently.

Well, that's up to the host, but in general, yes they will. So? (Do you think those two points are in conflict somehow, or what?)

This phrasing is a bit like serializing the name of the algorithm in a string.

A bit like that, yes, Conversely, having multiple job-enqueuing operations is a bit like baking the value of the jobGroup parameter into the operation-name. But neither perspective is really a reason for preferring one scheme over the other.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit like that, yes, Conversely, having multiple job-enqueuing operations is a bit like baking the value of the jobGroup parameter into the operation-name. But neither perspective is really a reason for preferring one scheme over the other.

I agree that you can argue this both ways. Here, I prefer the current phrasing because the strings tend to be constant, not treated as abstract. We just have to make judgement calls in these cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I prefer the current phrasing because the strings tend to be constant, not treated as abstract.

I.e., the first argument to a HostEnqueueJob would tend to be a 'string literal' as opposed to a metavariable whose value is a string? Yeah, it wouldn't surprise me if that turned out to be true.

@littledan littledan force-pushed the enqueue-promise-job branch 2 times, most recently from 6427d8b to 6d7c4dc Compare November 19, 2019 06:44
@syg syg force-pushed the enqueue-promise-job branch 2 times, most recently from b103c4c to e36a223 Compare March 5, 2020 01:31
@ljharb ljharb force-pushed the enqueue-promise-job branch 2 times, most recently from 3588286 to c595020 Compare March 9, 2020 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants