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

[css-layout-api] Generator vs. Promise design for the API #750

Open
bfgeek opened this issue Apr 9, 2018 · 5 comments
Open

[css-layout-api] Generator vs. Promise design for the API #750

bfgeek opened this issue Apr 9, 2018 · 5 comments

Comments

@bfgeek
Copy link
Contributor

bfgeek commented Apr 9, 2018

There isn't actually a large difference between the two APIs from just looking at the script, i.e.

  *layout(children) {
    const fragment = yield children[0].layoutNextFragment();
  }
  layout(children) {
    const fragment = await children[0].layoutNextFragment();
  }

There are various pros/cons of each of these.

Generator:

  • Pro: May be faster wrt. bindings.
  • Pro: More restrictive API, can't await on other APIs which might leak into the LWGS.
  • Con: Less obvious error handling to for web developers.
  • Con: Less obvious API as this pattern isn't common, and not many people use them (generators).

Promise:

  • Pro: More obvious error handling.
  • Pro: Common pattern for developers.
@css-meeting-bot
Copy link
Member

The Working Group just discussed Generator vs. Promise design for the API, and agreed to the following resolutions:

  • RESOLVED: We will continue adopting generators for layout functions.
The full IRC log of that discussion <dael> Topic: Generator vs. Promise design for the API
<dael> github: https://github.com//issues/750
<dael> iank_: From the author PoV...
<dael> Rossen: The only difference is you added a *
<dael> dbaron: You need an async in the second line.
<dael> iank_: True.
<dael> iank_: These are the pros and cons [futher down the issue]
<dael> iank_: If anyone else has something to add here, I'll come back to the next meeting with data.
<dael> Rossen: Are generators broadly impl?
<dael> iank_: Yes.
<dael> iank_: Everyone does generators, though they haven't been used like this before.
<dael> TabAtkins: We won't expose the functions you can't run.
<dael> iank_: We'll have to make sure in the future. We have to code with eyes open.
<dael> fremy: You like this better because you can only allow some things to be sent?
<dael> iank_: It makes a tighter API. Promises are more familiar.
<dael> fremy: Dev are familiar with converting in the background
<dael> dbaron: After thinking about this, I'm feeling like the way devs interface with the engine here feels better desc by generator and not promise.
<dael> dbaron: I think because promises have this tie to an event loop thing where promises are running async on this micro-task. You're in the middle of this thing in the layout engine and it needs to call you many times.
<dael> ??: I agree you're generating the list
<gsnedders> s/??/surma/
<dael> TabAtkins: You're not though. We can call you fresh.
<dael> surma: That's details. Conceptually it works as a generator.
<dael> surma: I'm wondering if there's a place where scheduling with rpomises may become and issue and it wouldn't wiht promises.
<dael> iank_: I think there's work in FF to work promises at the right time. For the syncronous engines we would all the layout engine, make sure task queue is exhausted, make sure we've got a promise.
<dael> surma: It makes more sense with generators even if it's 100% correct.
<dael> dbaron: I think one of the weird things about generator is it produces the last thing at the end. You're using yield and return syntax to do much deeper things.
<dael> TabAtkins: The yield at the top of the issue is not usefully yielding into the engine. It's saying do this layout and give me results. It's using a generator as an async iterator. It's possible, but not the preferred pattern.
<dael> TabAtkins: It off doing it's own thing. If it was synchronous we wouldn't have to do this.
<dael> surma: Both allow us to think about async and parallel layouts in the future?
<dael> iank_: Yeah.
<dael> Rossen: I'm hearing mostly people leaning word generators.
<dael> TabAtkins: No.
<dael> Rossen: Except TabAtkins.
<dael> iank_: We can also give two different versions and see what people expect
<dael> Rossen: TabAtkins would you object? Any objections?
<dael> TabAtkins: I have weak objection and would like wider review.
<dael> Rossen: Has there been review? TAG looked, did they say anything?
<dael> Rossen: I went through Travis's comments and they were high level.
<dael> dbaron: I don't remember this one.
<dael> iank_: I think notes from the TAG meeting are up.
<dael> [everyone hunts meeting notes from TAG]
<dael> dbaron: Minutes say "Travis: I have some review feedback to post to the issue."
<dael> dbaron: We came back to it the next day, though.
<dbaron> TAG minutes in https://github.com/w3ctag/meetings/blob/gh-pages/2018/04-tokyo/04-06-minutes.md
<dael> iank_: [skim-reads]
<dael> Rossen: There's a couple ways to get out. 1) We agree to adopt promise-based API 2) We stick with generators with a light objection from TabAtkins and then he can figure out if we should avoid it. If he comes back with reasons and we can change.
<dael> TabAtkins: I'll bother Dominic
<dael> Rossen: Sticking with Generators will force the discussion.
<dael> Rossen: Do we have people that object to promise-based APIs?
<dael> TabAtkins: Performance aspect
<dael> iank_: I want to do some benchmarking
<dael> iank_: I think layout will be particularly sensitive to bindings.
<dael> TabAtkins: My hope is we don't need to re-apply promises.
<dael> iank_: We might need special APIs.
<dael> iank_: I'll have everything hopefully done.
<dael> Rossen: Prop: We will continue adopting generators for layout functions. TabAtkins will followup
<dael> RESOLVED: We will continue adopting generators for layout functions.

@tabatkins
Copy link
Member

(Note that the resolution was explicitly meant to require discussion, not to shut down the debate and settle on one or the other.)

@dbaron
Copy link
Member

dbaron commented Apr 27, 2018

A colleague noted the relationship of what's being done conceptually here to coroutines, although I'm not sure if that yields anything useful. (I vaguely recall that Ian might have mentioned that at the face-to-face meeting as well, though I don't see it minuted.)

@tabatkins
Copy link
Member

Yeah, JS's iterator/yield and async/await are basically one-sided coroutines.

@css-meeting-bot
Copy link
Member

The Working Group just discussed Generator vs. Promise design for the API, and agreed to the following:

  • RESOLVED: use Promises
The full IRC log of that discussion <fantasai> Topic: Generator vs. Promise design for the API
<fantasai> github:
<fantasai> github: https://github.com//issues/750
<fantasai> iank_: Did some benchmarking
<fantasai> iank_: The Promise solution is slightly slower, but not that much slower
<fantasai> iank_: That makes me lean towards Promises solution
<fantasai> iank_: Advantage of gnerator is that it's faster, don't have to kick the ? every time
<fantasai> iank_: there's some overhead for doing that
<fantasai> iank_: However, I think that's acceptable from an author expectation perspective
<fantasai> iank_: If it was 30-40% overhead, that might be a concern
<fantasai> iank_: but turned out to be not as much of a problem
<fantasai> Rossen: What did you test?
<fantasai> iank_: Test was 100 custom layout elements
<fantasai> iank_: went about 6 layers deep, and had two child nodes at each level and 4 leaf nodes
<fantasai> iank_: tree structure, roughly 100 nodes
<fantasai> iank_: used our perf testing framework
<fantasai> iank_: I did a synchronous version of the API, didn't have any generators, promises, etc. Just exectued synchornously
<fantasai> iank_: That was about 650 runs per second
<fantasai> iank_: Our current impl was about 430 runs per second
<fantasai> iank_: so 50% off the synchronous one
<fantasai> iank_: Promises was 450 runs per second
<fantasai> iank_: Might be able to get it faster, push around 500 or something
<fantasai> iank_: We do lose a lot of perf by allowing async here
<fantasai> iank_: so something else to consider
<fantasai> iank_: One thing we could revisit later is, asnchornous layout engines
<fantasai> iank_: we could potentially get perfr by exposing synchronous versions of layout APIs
<fantasai> iank_: so leave that door open
<fantasai> iank_: I'll need to do some work, spec wise, to get it so that an impl can run this synchronously
<fantasai> iank_: Still requres a queue to go thorugh layout requests
<fantasai> iank_: Once I've got that written down, make sure it makes sense to everyone
<fantasai> frremy: What if you await Promise that never returns?
<fantasai> iank_: IWhen you call layoutNextFragment(), pushes a request onto an internal queue. If layout engine has exhausted that queue
<fantasai> iank_: It'll keep queuing its own tasks, flush that queue and echeck if resolved prmise is done
<fantasai> iank_: If ....
<fantasai> iank_: Summary of the engine is, layoutnextfragment), pushes onton internal queue for custom layotu instance. Layout engine will enqueue a microtask.
<fantasai> iank_: lay everything out and then queue tiself again in case extra work
<fantasai> iank_: if it finishes, then... if promis is resolved, then done
<fantasai> Rossen: So, resolution is to switch back to Promises
<fantasai> Rossen: Any additional comments or objections?
<fantasai> eae: Can we add a note about err-handlign expectations?
<fantasai> ACTION: iank to add note about error-handling expectations
<trackbot> Error finding 'iank'. You can review and register nicknames at <https://www.w3.org/Style/CSS/Tracker/users>.
<fantasai> RESOLVED: use Promises
<fantasai> (and add note)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants