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

Fixes #335: Prevent complex DOM workloads from hitting a specific WebKit pathology #338

Merged
merged 3 commits into from
Dec 21, 2023

Conversation

issackjohn
Copy link
Contributor

@issackjohn issackjohn commented Dec 19, 2023

The PR Introduces a new variant of the Complex DOM shell.

Fixes #335.

big-dom.css is the original complex DOM shell and this new variant - big-dom-with-stacking-context-scrollable.css - is an introduction of the CSS property isolation: isolate to the .tree-area.

The purpose of this variant is to is to vary complex DOM contents across the different subtests so as to avoid hitting the same pathology in every complex DOM workload.

Performance Measurements

${\textsf{\color{green}Green}}$: Running In Complex Setting

${\textsf{\color{lightblue} lightblue}}$: Running in Stand-alone Setting

We only report the performance results for the six TodoMVC tests that we run in the complex setting as well as the four tests that we run in the stand-alone setting but have been changed to use the new variant(big-dom-with-stacking-context-scrollable.css)
${\textsf{\color{green} }}$

TodoMVC Test big-dom.css (Original) big-dom-with-stacking-context-scrollable.css
${\textsf{\color{green}TodoMVC-JavaScript-ES6-Webpack-Complex-DOM}}$ -
${\textsf{\color{green}TodoMVC-Lit-Complex-DOM}}$ -
${\textsf{\color{green}TodoMVC-Preact-Complex-DOM}}$ -
${\textsf{\color{green}TodoMVC-Svelte-Complex-DOM}}$ -
${\textsf{\color{green}TodoMVC-React-Complex-DOM}}$ -
${\textsf{\color{green}TodoMVC-Angular-Complex-DOM}}$ -
${\textsf{\color{lightblue}TodoMVC-JavaScript-ES5-Complex-DOM}}$ -
${\textsf{\color{lightblue}TodoMVC-WebComponents-Complex-DOM}}$ -
${\textsf{\color{lightblue}TodoMVC-React-Redux-Complex-DOM}}$ -
${\textsf{\color{lightblue}TodoMVC-Backbone-Complex-DOM}}$ -
${\textsf{\color{lightblue}TodoMVC-Vue-Complex-DOM}}$ -
${\textsf{\color{lightblue}TodoMVC-jQuery-Complex-DOM}}$ -
Screenshot 2023-12-20 at 5 07 41 PM

Hosted at:

https://issackjohn.github.io/Speedometer5/

* Utility function to create css variants for big-dom-generator css
* Introduce v2 variants
@issackjohn issackjohn marked this pull request as ready for review December 19, 2023 17:27
@rniwa
Copy link
Member

rniwa commented Dec 19, 2023

I guess you left the ones we don't use complex DOM setup (i.e. only runs in non-complex setup) to be v1?? Is that what's happening here?

@issackjohn
Copy link
Contributor Author

I guess you left the ones we don't use complex DOM setup (i.e. only runs in non-complex setup) to be v1?? Is that what's happening here?

Yes that's correct.

@rniwa
Copy link
Member

rniwa commented Dec 19, 2023

I guess you left the ones we don't use complex DOM setup (i.e. only runs in non-complex setup) to be v1?? Is that what's happening here?

Yes that's correct.

Can we switch those to v2 as well so that when we enable them in developer menu, we'd get a meaningful measurement?

@sulekhark
Copy link
Contributor

sulekhark commented Dec 19, 2023

I guess you left the ones we don't use complex DOM setup (i.e. only runs in non-complex setup) to be v1?? Is that what's happening here?

Yes that's correct.

Can we switch those to v2 as well so that when we enable them in developer menu, we'd get a meaningful measurement?

@rniwa, would it make sense to keep two of the six (which only run in non-complex setup) to be v1 and the rest four v2? Just to be able to see the impact in the developer mode too? Maybe JQuery (or, Vue) and WebComponents with v1? WDYT?

@flashdesignory
Copy link
Contributor

Is there a way to make the choice between v1 / v2 dynamic?
Ideally the test suits would tell the complex dom which version to use in my opinion.

@sulekhark
Copy link
Contributor

Is there a way to make the choice between v1 / v2 dynamic? Ideally the test suits would tell the complex dom which version to use in my opinion.

@flashdesignory,
In the current design, the Complex DOM TodoMVC tests are built by embedding the pre-built TodoMVC dist in a pre-built Complex DOM shell (built in the big-dom-generator folder). In the last-but-one working group meeting which Simon Fraser attended, I remember we discussed that we would need to have a Complex DOM shell 1 and a Complex DOM shell 2. Keeping these two things in mind, we chose to take the approach in this PR.
I think that what you suggest is possible, but it would need quite some code refactoring. If in the future, we see the need to frequently change the embedding of Complex DOM TodoMVC tests from one shell to another (even in the dev environment), we could make this embedding dynamic (i.e. as you said, specified by the test). WDYT?

@flashdesignory
Copy link
Contributor

@sulekhark - sounds good 👍

@bgrins
Copy link
Contributor

bgrins commented Dec 20, 2023

I'll let @julienw review, but I took a quick look and it's a relatively small change. Other than the file naming issue in the inline comment I don't have any particular concerns.

I don't have a particular preference on the topic of which variant the non-enabled tests should use. We could always push preview branches in alternative configurations for testing locally & in CI. If I had to choose I suppose I would keep the current (non-isolation) variant as the default for non-enabled, since Lit & Web Components have unique styles from the other tests, so that would maintain the Web Components test as the only way to run (webcomponent style + non isolation).

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

I left a few comments, with "request changes" because I'd like to see the result of the changes.

Thanks

* use dictionary instead
* rename css
* update names in build scripts
* build all dists
* improve error message
@issackjohn issackjohn requested a review from julienw December 20, 2023 17:42
@issackjohn
Copy link
Contributor Author

Screenshot 2023-12-20 at 10 16 55 AM

@sulekhark
Copy link
Contributor

sulekhark commented Dec 20, 2023

I guess you left the ones we don't use complex DOM setup (i.e. only runs in non-complex setup) to be v1?? Is that what's happening here?

Yes that's correct.

Can we switch those to v2 as well so that when we enable them in developer menu, we'd get a meaningful measurement?

@rniwa, would it make sense to keep two of the six (which only run in non-complex setup) to be v1 and the rest four v2? Just to be able to see the impact in the developer mode too? Maybe JQuery (or, Vue) and WebComponents with v1? WDYT?

Confirmed with Ryosuke Niwa (via a 1:1 chat on slack) that the above proposed plan is good. So, for the todo tests that are not enabled in the complex setting Vue and WC will have variant 1 and Vanilla JS5, JQuery, Backbone, React-Redux will have Variant 2 - this will kick-in only in developer mode.

Here's the final configuration (with Variants 1 and 2 renamed as suggested by all):

  • Todo tests enabled in complex setting:
    - Variant 1: React, webpack
    - Variant 2: angular, lit, preact, svelte
  • Todo tests not enabled in complex setting:
    - Variant 1: Vue, WC
    - Variant 2: JQuery, React-Redux, Backbone, Vanilla JS5

* update build scripts
* update dists
Copy link
Contributor

@julienw julienw 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, thanks

@issackjohn issackjohn requested a review from rniwa December 21, 2023 15:16
@issackjohn
Copy link
Contributor Author

Thank you all!

@issackjohn issackjohn merged commit a268ec4 into WebKit:main Dec 21, 2023
4 checks passed
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.

Every complex DOM workloads should not hit a specific WebKit pathology
7 participants