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

Layering: fix the jobs infrastructure #735

Closed
wants to merge 1 commit into from
Closed

Conversation

domenic
Copy link
Member

@domenic domenic commented Nov 18, 2016

This implements the proposal discussed in #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.


This surely needs some editorial work and collaboration, but it's a start.

This is strictly a layering change. (Or at least is meant to be, modulo mistakes.) It does not lessen or increase the requirements on hosts:

  • As per discussions in ScriptEvaluationJob and TopLevelModuleEvaluationJob seem to be useless; let's remove them #240 hosts are allowed to take advantage of the many implementation-defined sections of EnqueueJob to gain freedom equivalent to the new HostEnqueueJob. (Check me on this that I didn't miss any requirements!)
  • Hosts were never required to use RunJobs or its dependencies TopLevelModuleEvaluationJob and ScriptEvaluationJob; they were just kind of thrown into the specification as dead-code abstract operations disconnected from the rest of the spec. And indeed, as discussed at length previously, it is impossible for realistic hosts to use them as-is. So moving them to an informative annex as a sample host environment does not change any normative requirements.

@domenic
Copy link
Member Author

domenic commented May 1, 2017

Rebased on master.

It's worth mentioning that this not being merged confused Chrome implementers working on modules, as they didn't realize the spec did not match browser implementations, and had to be redirected to my fork to understand how it was actually implemented.


<p>Every ECMAScript implementation has at least the Job Queue `"PromiseJobs"`, which are responses to the settlement of a Promise (see <emu-xref href="#sec-promise-objects"></emu-xref>).</p>

<p>A request for the future execution of a Job is made performing HostEnqueueJob, giving a Job Queue name, a Job abstract operation, and any necessary argument values. At some time when there is no running execution context and the execution context stack is empty, the ECMAScript implementation will perform the givne Job abstract operation and evaluate it with the given arguments.</p>
Copy link
Member

Choose a reason for hiding this comment

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

typo: givne

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed!


<emu-annex id="sec-example-module-resolution">
<h1>Module Resolution</h1>
<p>This implementation uses the [[HostDefined]] field of Source Text Module Records to store the filename from which a module was retrieved. It also maintains a List of ModuleMap Records, known as the module map. This helps enforce the idempotency requirements of HostResolveImportedModule. ModuleMap Records have the fields described here:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative solution here to avoid having to use Filename (which sounds like some alien to this specification) is to create a caching mechanism based on the module specifier. We played around with this recently, and seems to be sufficient to maintain the idempotency requirements. It will be very similar to what you have here, but just mapping between the module specifier (per dep) and the corresponding module record, and using the Filename only if the record is not found in the map.

This might be important for the future when we introduce a way to create other kind of records, in which case we might want to pre-link them before they are instantiated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is just an example annex, I think it's OK to talk about filenames.

1. Let _filename_ be the result of resolving _specifier_ against _referencingFilename_ as a filename, according to operating system conventions.
1. If the module map contains a ModuleMap Record with [[Filename]] equal to _filename_, return that Record's [[Module]] field.
1. Let _sourceText_ be the result of using the operating system's mechanisms to read the source text of the file located at _filename_. If this fails (e.g. because the file does not exist), throw a *TypeError*.
1. Let _moduleRecord_ be ? ParseModule(_filename_).
Copy link
Contributor

Choose a reason for hiding this comment

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

ParseModule is confusing here since it is not the abstract operation defined in the specification.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is indeed pretty buggy. Let me fix.

<emu-annex id="sec-example-initialization">
<h1>Initialization and Ongoing Evaluation</h1>

<p>This implementation is initialized by being provided with a set of ECMAScript source texts for zero or more scripts, and zero or more modules; the modules must be provided alongside corresponding filenames. It then perofrmsthe following algorithm to evaluate those source texts in appropriate Jobs. If evaluation enqueues any further Jobs, such as promise-related Jobs, the call back to the implementation's HostEnqueueJob abstract operation will ensure those are executed after the ongoing evaluation of the script or module.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

perofrmsthe => performs the

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.
@littledan
Copy link
Member

I think it'd be great if this effort moved forwards, given the clarity issues with the current specification that @domenic noted in #735 (comment) . Does anyone have concerns with this direction?

@littledan
Copy link
Member

littledan commented Feb 10, 2019

I'd like to give this PR a ping. As I'm looking into the top-level await and WeakRefs specifications, it keeps coming up that the lack of alignment requires the same thing to be specified twice--in HTML and in RunJobs. It's not clear how to write tests against the RunJobs version, as I don't know any implementations of it.

I understand the argument that this patch may be omitted while still corresponding to some possible other HTML change in favor of various complicated interpretations of the specification, but it's very hard for me to understand the correspondence when reading it, and I doubt I'm the only one. I prefer the style of explicit host hooks, as this PR provides.

Are there any JavaScript environments which implement RunJobs as specified in the current specification? Would it work for them to define their semantics in terms of the example given here? Or should we make some other kind of tool to help them define their job queue semantics more easily?

What's the process for moving forward here? Do the editors plan to review this patch, or does it need to be brought to a TC39 meeting to request a review?

@annevk
Copy link
Member

annevk commented Feb 10, 2019

I suspect this is a problem for Atomics.waitAsync() too: tc39/proposal-atomics-wait-async#6. (Though that might also hit the problem of agents <> hosts not being great.)

@erights
Copy link

erights commented Feb 15, 2019

What's the process for moving forward here? Do the editors plan to review this patch, or does it need to be brought to a TC39 meeting to request a review?

This definitely needs attention (presentation and discussion) at a tc39 meeting. The JS job scheduling model must be meaningful across a range of host environments, definitely including the browser.

@littledan
Copy link
Member

littledan commented Feb 16, 2019

@erights That's a great goal. It's unfortunate that it's not currently meaningful in the browser. I believe Node.js's behavior is somewhat similar to the browser's with respect to job queues (cc @joyeecheung). Do you know where I could learn more about other host environments' requirements? cc @patrick-soquet

@devsnek
Copy link
Member

devsnek commented Feb 16, 2019

node and chrome use the same job lifecycle (although slightly different apis from v8, node manually calls RunMicrotasks, while chrome uses microtask scopes)

@joyeecheung
Copy link

joyeecheung commented Feb 19, 2019

In Node.js, the scheduling of RunMicrotasks is done in JavaScript to reduce the boundary-crossing overhead, and it could call RunMicrotasks (probably more or less corresponds to the current RunJobs but no TopLevelModuleEvaluationJob or ScriptEvaluationJob are taken care of) multiple times in one tick, which seems to contradict with

The evaluation of any Jobs enqueued via previous calls to HostEnqueueJob with the same queueName argument must complete before evaluation of the Job starts.

described in HostEnqueueJob here.

The current logic is captured in

https://github.com/nodejs/node/blob/d18b0a01320e392e48f0c475effd8d4db2bb71a8/lib/internal/process/next_tick.js#L45-L84

@domenic
Copy link
Member Author

domenic commented Feb 19, 2019

The intent there is to prevent multithreading of jobs in parallel. Can you think of a clearer way to frame that which would avoid confusion?

@joyeecheung
Copy link

@domenic The implementation-defined bits here seem to be the way the jobs are evaluated: either one at a time (descried in HostEnqueueJob here), or batched in one go with implementation-defined operations interleaved (in Node.js). For Node's cases, the current requirements in HostEnqueueJob seems too granular. In Node.js the guarantee that no jobs can be run in parallel comes from the run-to-completion of the scheduling function written in JS.

Maybe adding the loop from RunJobs to HostEnqueueJob, replacing

  1. Perform the abstract operation given by job, passing the arguments arguments.

could be a workaround? It seems reasonable to assume that jobs in the same queue should be evaluated sequentially in the same thread until it's empty.

@domenic
Copy link
Member Author

domenic commented Feb 19, 2019

No, that gets us back to the original problem this PR is trying to address, where algorithmic descriptions are overconstraining on hosts (e.g. don't allow appropriate interleaving of other steps).

Can you think of any way to say "don't execute jobs in parallel threads" that avoids the confusion you encountered from the original framing?

@devsnek
Copy link
Member

devsnek commented Feb 19, 2019

the spec is aware of threads right? maybe we can just put that prose in the spec?

@littledan
Copy link
Member

I'd like to understand a bit better what the problems are with this PR. @erights talked about job scheduling being meaningful across environments. Were there any guarantees that we might want that aren't included in the PR in https://github.com/tc39/ecma262/pull/735/files#diff-3540caefa502006d8a33cb1385720803R6154 ?

@joyeecheung For context, this isn't about some tweak needed around the edges, but the fact that HTML does not use this job queue algorithm at all, see https://html.spec.whatwg.org/#integration-with-the-javascript-job-queue for details. I like the idea of specifying job scheduling in the JS spec, but it's not clear how to integrate that with all the things HTML does, without a ton of host hooks which would be much more confusing than this PR, and end up expressing the same kinds of invariants as the PR already makes explicit.

@syg
Copy link
Contributor

syg commented Mar 23, 2020

Closing this in light of #1597 landing. @domenic @littledan Please re-open if #1597 didn't in fact subsume this PR, which is my current understanding.

@syg syg closed this Mar 23, 2020
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.

9 participants