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

Coalesce all translations from MutationObserver into a single rAF #113

Merged
merged 1 commit into from Jan 12, 2018

Conversation

zbraniecki
Copy link
Collaborator

This changes the way we react to mutations by batching all localizations to happen once per animation frame irrelevant of the number of microtasked mutations fired.

@stasm
Copy link
Contributor

stasm commented Dec 20, 2017

I need some context here :) What is this for? Does it make things faster? By how much? How did you test? Do you want me to prioritize this over 0.5?

@zbraniecki
Copy link
Collaborator Author

I need some context here :)

Oh, stupid me. Sorry! Here it is: https://bugzilla.mozilla.org/show_bug.cgi?id=1389384

What is this for?

Gecko and Fluent-DOM

Does it make things faster?

It makes us do less things.

How did you test?

I created a set of tests that synchronously and asynchronously updated UI in Preferences of Gecko and monitored the number of mutations and translations we perform.

With the patch we perform one batch per animation frame at most. Without it, we are happy to perform hundreds per millisecond if needed.

Do you want me to prioritize this over 0.5?

I'd hope you can evaluate the code. The DOM API integration review is handled by :mossop in the bug. If you think you're to busy with other things I honestly believe that his review should be enough.

@zbraniecki
Copy link
Collaborator Author

I got r+ from :mossop in https://bugzilla.mozilla.org/show_bug.cgi?id=1389384

@stasm do you want me to wait for you before landing this?

@stasm
Copy link
Contributor

stasm commented Jan 11, 2018

No, please don't wait; go ahead and land.

@zbraniecki zbraniecki merged commit add0c1f into projectfluent:master Jan 12, 2018
@stasm
Copy link
Contributor

stasm commented Jan 12, 2018

@zbraniecki How hard would it be to have tests for this?

@zbraniecki
Copy link
Collaborator Author

I have no idea how to write tests that trigger the behavior I'm testing this on. Karma? I need to trigger tons of mutations per animation frame and verify I get just one translate elements out of it.

@zbraniecki zbraniecki deleted the dom-coalesce branch May 14, 2018 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants