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

Tie event loops to agents #4214

Merged
merged 9 commits into from
Feb 7, 2019
Merged

Tie event loops to agents #4214

merged 9 commits into from
Feb 7, 2019

Conversation

annevk
Copy link
Member

@annevk annevk commented Nov 28, 2018

@annevk
Copy link
Member Author

annevk commented Nov 28, 2018

I'm looking for feedback on approach and how to tackle specific things I called out in comments.

@annevk annevk requested a review from domenic November 28, 2018 13:17
@domenic
Copy link
Member

domenic commented Nov 28, 2018

Hmm, I'm worried this might not align well with our desire to give implementations more freedom. Maybe what I'm worried about is more relevant for agent clusters, but I thought it was possible (and indeed likely, in many-tab situations) for multiple agents to share an event loop.

@annevk
Copy link
Member Author

annevk commented Nov 30, 2018

Since that difference isn't observable, I think it's better for the design to aim for maximum concurrency.

We could add a note, but it might get complicated given JavaScript's expectations around forward progress for agents.

@annevk annevk force-pushed the annevk/event-loop-refactoring branch from 11c75f2 to adc60d7 Compare January 11, 2019 11:11
@domenic
Copy link
Member

domenic commented Jan 17, 2019

Thoughts so far:

  • It's not clear why this needs to be 1:1. I can't find the part of the spec that makes use of that property. A smaller delta would be to allow many agents -> 1 event loop. You can still go from agent -> event loop that way, which is what I see happening in this patch.

  • I think it's useful to have terms like "window event loop" or "worker event loop" still instead of "an event loop for a similar-origin window agent". Indeed, introducing dedicated <dfn>s for them would be an improvement over the status quo where the terms "browsing context" and "event loop" are linked separately. I guess you might also want "worklet event loop".

  • The new "we don't describe how to handle this" note is scarier than the old one. Upon consideration I guess it's the same? It'd be good to flesh out an example. I suspect this is a very common case, so showing it (e.g. "the user navigates from URL X to Y within a single browsing context. The spec doesn't handle that") would be helpful.

@annevk
Copy link
Member Author

annevk commented Jan 18, 2019

I guess we could allow sharing, but it seems that would just make things more complicated, as it's not as simple as allowing any two agents to share, that won't work. Sharing it across origins is rather questionable given Spectre. Sharing it within an agent cluster is not allowed by JavaScript. Sharing it across browsing context groups would not allow for making background tabs use less CPU, but could be allowed if user agents don't care for that. Is that the one you want?

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Jan 24, 2019
@domenic
Copy link
Member

domenic commented Jan 24, 2019

Per https://freenode.logbot.info/whatwg/20190118#c1931283 it looks like we want to take a slightly different approach here.

Event loops ought to be 1:1 with agents, let's make it so.

Helps with #4198.
@annevk annevk force-pushed the annevk/event-loop-refactoring branch from adc60d7 to 9339d99 Compare January 29, 2019 16:36
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk annevk changed the title Make event loops 1:1 with agents Tie event loops to agents Feb 1, 2019
@annevk annevk added topic: event loop and removed do not merge yet Pull request must not be merged per rationale in comment labels Feb 1, 2019
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Great stuff, looking pretty close. I've tried to be concrete about there is left to do.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk
Copy link
Member Author

annevk commented Feb 6, 2019

I carefully checked all outstanding feedback and I think I got it covered with the latest commit.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with some remaining nits which I trust you to resolve. Thanks again for the patience.

source Show resolved Hide resolved
source Show resolved Hide resolved
@annevk annevk merged commit f97d629 into master Feb 7, 2019
@annevk annevk deleted the annevk/event-loop-refactoring branch February 7, 2019 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants