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

Terminology: AsyncContext mapping/store/snapshot #73

Open
andreubotella opened this issue Feb 27, 2024 · 9 comments
Open

Terminology: AsyncContext mapping/store/snapshot #73

andreubotella opened this issue Feb 27, 2024 · 9 comments

Comments

@andreubotella
Copy link
Member

In the spec, the key-value map data structure mapping AsyncContext.Variable objects to their values is called the "Async Context Mapping Record". However, the various fields in which it's stored have different names: [[AsyncContextMapping]], [[AsyncContextSnapshot]], [[AsyncSnapshotMapping]]... We (the various folks working on this proposal) have also sometimes referred to this data structure as the "AsyncContext store". I think we should decide if we keep the name "AsyncContext mapping", and change the various field names to use it.

(This issue is not about renaming the AsyncContext.Snapshot API.)

@jridgewell
Copy link
Member

jridgewell commented Feb 27, 2024

The different [[…]] names were intentional, so that types can't be used interchangeably. Eg, snapshot.run.call(asyncVar, …) should fail because they have different fields. We could standardize on calling all of these …Store, as long as the is unique between types.

@littledan
Copy link
Member

Let's stick to "snapshot" for all three of these, since they represent the same thing. Or, we could change the JS-level API, and then use that name for all of them.

@Qard
Copy link

Qard commented Apr 8, 2024

WinterCG calls that internal thing AsyncContextFrame, for reference.

@littledan
Copy link
Member

WinterCG calls that internal thing AsyncContextFrame, for reference.

Note that the use of that term, "Async Context Frame", is entirely contained in that one document. It shouldn't be taken any more seriously as a "precedent" than the current spec text.

@Qard
Copy link

Qard commented Apr 11, 2024

Yep, just sharing an example of a way it has been done before in case that is relevant to the naming, but also possibly relevant to the structure in that AsyncContextFrame combines multiple parts of what AsyncContext does into one place.

The frame approach is especially an advantage because it means the most common case of propagating is made into only a pointer copy while the far less common case of an individual store.run(...) does a small bit of extra work to produce a new frame rather than only updating that store instance. This provides multi-tenancy without O(m x n) cost where m is store count and n is async transition count. It's still O(m x n) but over run counts which should generally never exceed async transition counts and should typically be far less.

Anyway, that's a bit of an aside to the terminology point, but I think what should be said is the terminology is part reflection of the implementation and part reflection of the intent.

@littledan
Copy link
Member

What is the counterargument to using "snapshot" everywhere?

@Qard
Copy link

Qard commented Apr 12, 2024

It does the same thing as Snapshot, but without needing separate Mapping, Storage, and Revert types. It works by treating the "frame" as immutable, making a clone whenever it would be changed, which happens far less frequently than when a snapshot needs to happen as you need a snapshot every time it propagates. The frame design means propagation is only a copy of the reference to what the current context frame is. A "snapshot" then becomes just a copy of the pointer to the current frame, which is very fast.

The API as it is presently seems a lot more complicated than it actually needs to be. It seems to be optimizing for making writes fast when reads are typically thousands of times more frequent.

@jridgewell
Copy link
Member

Based on today's meeting, we'll switch to consistently using "mapping" term internally.

@legendecas

This comment was marked as resolved.

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

No branches or pull requests

5 participants