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

Automatic Message Batching #2421

Merged
merged 5 commits into from Feb 7, 2022
Merged

Conversation

futursolo
Copy link
Member

@futursolo futursolo commented Jan 29, 2022

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)

PR      master  example
416K	444K	examples/boids
312K	360K	examples/contexts
236K	252K	examples/counter
256K	280K	examples/dyn_create_destroy_apps
268K	288K	examples/file_upload
436K	552K	examples/function_memory_game
456K	512K	examples/function_todomvc
488K	504K	examples/futures
296K	312K	examples/game_of_life
224K	240K	examples/inner_html
240K	256K	examples/js_callback
432K	456K	examples/keyed_list
240K	256K	examples/mount_point
332K	348K	examples/nested_list
248K	268K	examples/node_refs
2.1M	2.1M	examples/password_strength
264K	276K	examples/portals
828K	996K	examples/router
308K	340K	examples/suspense
252K	268K	examples/timer
368K	384K	examples/todomvc
240K	252K	examples/two_apps
248K	264K	examples/webgl

Requires #2420 (due to messing up git history)

Checklist

@github-actions
Copy link

github-actions bot commented Jan 29, 2022

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 🌎

@futursolo
Copy link
Member Author

Hmm, the tests need to become async as well.

@futursolo futursolo marked this pull request as draft January 29, 2022 05:44
@futursolo futursolo marked this pull request as ready for review January 29, 2022 07:56
@voidpumpkin voidpumpkin added A-yew Area: The main yew crate A-yew-router Area: The yew-router crate labels Jan 30, 2022
@futursolo
Copy link
Member Author

@voidpumpkin Labeling should be -A-yew-router +breaking change?

@hamza1311 hamza1311 added breaking change and removed A-yew-router Area: The yew-router crate labels Jan 30, 2022
@hamza1311
Copy link
Member

@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.

@futursolo
Copy link
Member Author

Oh, I think the entirety of the work on this branch has been destroyed.

@hamza1311
Copy link
Member

Oh wow, that's rough. Do you also not have any local copy of the work?

@futursolo
Copy link
Member Author

This is stupid. But I managed to recover the work from Time Machine.

@futursolo futursolo reopened this Jan 30, 2022
@futursolo
Copy link
Member Author

This branch should have been restored and is ready for review.

Copy link
Member

@hamza1311 hamza1311 left a 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.

packages/yew/src/html/component/lifecycle.rs Show resolved Hide resolved
packages/yew/src/scheduler.rs Show resolved Hide resolved
packages/yew/tests/suspense.rs Show resolved Hide resolved
@futursolo
Copy link
Member Author

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:

https://github.com/yewstack/yew/pull/2421/files#diff-d12f7bf5d9bc0713eebab16b0d6c45dfad36a381f46639c372171b713a3a77aeR19

@voidpumpkin
Copy link
Member

voidpumpkin commented Jan 31, 2022

@futursolo You changed yew-router and yew files so imo it needs both those labels.
The breaking change tag imo is useless atm so I tend to forget to apply it.
Our changelog generation currently does not utilize that tag.

cc @hamza1311

@futursolo
Copy link
Member Author

futursolo commented Jan 31, 2022

@futursolo You changed yew-router and yew files so imo it needs both those labels.

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?

Copy link
Member

@voidpumpkin voidpumpkin left a 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.

Copy link
Member

@hamza1311 hamza1311 left a 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;

Comment on lines +32 to +54
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
}
}
Copy link
Member

@hamza1311 hamza1311 Feb 7, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants