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

Refactor SynchronizationContext to provide order and execution guarantees #5002

Merged
merged 11 commits into from Jan 26, 2022

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Jan 25, 2022

No description provided.

@peppy
Copy link
Sponsor Member Author

peppy commented Jan 25, 2022

Needs further consideration. Dunno how this is supposed to work.

`Send` is supposed to block until the work completes (and run all
previous work).

`Post` is supposed to fire-and-forget.

Both cases need to ensure correct execution order.
@peppy
Copy link
Sponsor Member Author

peppy commented Jan 25, 2022

I've updated the implementation to match expectations of Windows Form / WPF in the table on https://docs.microsoft.com/en-us/archive/msdn-magazine/2011/february/msdn-magazine-parallel-computing-it-s-all-about-the-synchronizationcontext. I think this is closest to what we want.

Note that as a result, the Send method is running through the scheduler queue. If this is seen as dangerous (due to a potential expectation that it is to be run at precisely the start of the update method - I'm not sure how much water this holds) then we can make an isolated scheduler or local queue for the SynchronizationContext implementation.

I'd recommend reviewers do their own reading on the topic, the linked page above is worth a read, potentially twice over.

I'll add tests (covering order of execution and inline execution constraints) once we come to an agreement.

@smoogipoo
Copy link
Contributor

Is there a reason for Send to run through the scheduler queue? I see nothing in that doc which indicates that this is a guarantee of SynchronizationContext.

@peppy
Copy link
Sponsor Member Author

peppy commented Jan 25, 2022

Relevant sections:

2EE66243-8951-431A-9747-92BA1E66DA1D

I propose we match WPF/winforms for sanity, and because correct ordering is required for the realm case I'm making work with this change.

@smoogipoo
Copy link
Contributor

smoogipoo commented Jan 25, 2022

I guess we have different readings for what "execute in queue order" means.

It doesn't look like the WPF dispatcher preserves ordering between Post and Send when looking at the source. For example, their Send implementation eventually runs into a "fast path" which runs the callback immediately. Or you could look at the documentation of DispatcherPriority.Send which they're passing in through this particular pathway in the Send method, which states: "Operations at this priority are processed before other asynchronous operations".

@smoogipoo smoogipoo requested a review from bdach January 25, 2022 13:22
@peppy
Copy link
Sponsor Member Author

peppy commented Jan 25, 2022

Those both seem dependent on a priority enum though. Is the Send priority a default in the case of syncContext.Send?

Regardless, I wouldn't want to get that complex. As per the MS documentation, implementation is up to the implementation, and enforcing order isn't going to break anything.

@smoogipoo
Copy link
Contributor

Put another way, I definitely feel uneasy running every single scheduled delegate immediately.

@peppy
Copy link
Sponsor Member Author

peppy commented Jan 25, 2022

I don't necessarily disagree. Would you be okay if it was isolated from the game thread scheduler? That would remove any uneasiness from my end.

@smoogipoo
Copy link
Contributor

Yes, that would be fine

@smoogipoo
Copy link
Contributor

Those both seem dependent on a priority enum though. Is the Send priority a default in the case of syncContext.Send?

To answer this question, it seems like the default priority is Normal: https://source.dot.net/#WindowsBase/System/Windows/Threading/Dispatcher.cs,1784. The Send priority is a very special case which allows only those particular callbacks to be executed immediately (but not the rest of the queue).

@pull-request-size pull-request-size bot added size/L and removed size/S labels Jan 25, 2022
@peppy
Copy link
Sponsor Member Author

peppy commented Jan 25, 2022

Tests and logic should be good to go.

@peppy peppy removed the blocked label Jan 25, 2022
@peppy peppy changed the title Allow SchedulerSynchronizationContext to run work inline when feasible Refactor SynchronizationContext to provide order and execution guarantees Jan 25, 2022
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

I don't have a huge amount of knowledge about sync contexts but this pretty much matches what I'd expect to see out of one.

One thing I'm not entirely clear on is when the game will actually use Send(), if at all. But maybe that will become clear when I look at that part.

osu.Framework/Threading/Scheduler.cs Outdated Show resolved Hide resolved
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
@bdach bdach requested a review from smoogipoo January 25, 2022 19:42
@smoogipoo smoogipoo merged commit 5ffc25f into ppy:master Jan 26, 2022
@peppy peppy deleted the sync-context-run-inline branch February 4, 2022 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants