-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Automatic Message Batching #2421
Conversation
Visit the preview URL for this PR (updated for commit 2ef8c2d): https://yew-rs--pr2421-concurrent-mode-e5tgtrkq.web.app (expires Mon, 07 Feb 2022 02:53:58 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Hmm, the tests need to become async as well. |
@voidpumpkin Labeling should be -A-yew-router +breaking change? |
@futursolo do you want to rebase to fix the git history or is it fine as-is? It's going to get squashed anyway but I'm wondering if you want to rebase. |
7fb565d
to
b7b28ba
Compare
Oh, I think the entirety of the work on this branch has been destroyed. |
Oh wow, that's rough. Do you also not have any local copy of the work? |
This is stupid. But I managed to recover the work from Time Machine. |
This branch should have been restored and is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just these one comment that needs to be addressed
Labeling should be -A-yew-router +breaking change?
What's the breaking change here.
If one is expecting to send a message and see the effect immediately, then they will be "disappointed" (like in the tests). This might be a bigger change than one thinks. The migration documentation has explained this in detail: |
@futursolo You changed yew-router and yew files so imo it needs both those labels. cc @hamza1311 |
Should this label also apply if it only changed the test case to add yielding points and has no real changes on the code itself? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cant say am very "familiar with asynchronous IO",
but nice PR :D
I love the optimizations and the results of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Just wondering why these are needed:
sleep(Duration::ZERO).await;
pub fn push(&self, msg: Msg) -> usize { | ||
let mut inner = self.0.borrow_mut(); | ||
inner.push(msg); | ||
|
||
inner.len() | ||
} | ||
|
||
pub fn append(&self, other: &mut Vec<Msg>) -> usize { | ||
let mut inner = self.0.borrow_mut(); | ||
inner.append(other); | ||
|
||
inner.len() | ||
} | ||
|
||
pub fn drain(&self) -> Vec<Msg> { | ||
let mut other_queue = Vec::new(); | ||
let mut inner = self.0.borrow_mut(); | ||
|
||
std::mem::swap(&mut *inner, &mut other_queue); | ||
|
||
other_queue | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Deref/DerefMut impl is better than this. Not that it matters too much since this isn't exposed API
I misread. It's fine as is
Description
This is the first step towards concurrent mode.
When working on Bounce, a Yew state management library, I find that when a message is sent outside of a scheduled runner (e.g.: event handlers), it triggers a render immediately. Which can be inefficient when a component subscribes to many states.
Example:
master.mov
In the example, the components on the first row subscribe to 1 state,
the components on the second row subscribe to 2 states
and the component on the third row subscribes to 3 states.
As the video above demonstrated, currently, even if the updates are dispatched back-to-back, this still results in multiple renders for components with multiple states.
This can be solved by running updates in a
Runnable
but I think the better way to solve this issue is to give Yew (part of) concurrent mode so any messages that are sent in a blocking task can be batched automatically.This pull request makes Yew to automatically batch the messages for components and no longer triggers a render immediately.
The scheduler now runs at the next browser tick (or at the end of current tick if it is spawned by another future).
pr.mov
As a side effect, this allows most generics to be removed from the runners and hence all examples received a 10%~20% size reduction. (Some examples are now smaller than 0.19.3)
Requires #2420 (due to messing up git history)
Checklist
cargo make pr-flow
(pr-flow is broken on macOS #2403)