WebKit / Speedometer

An open source repository for the Speedometer benchmark
Other
592 stars 70 forks source link

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

Closed issackjohn closed 8 months ago

issackjohn commented 9 months ago

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/

rniwa commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

@sulekhark - sounds good 👍

bgrins commented 9 months ago

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

issackjohn commented 9 months ago
Screenshot 2023-12-20 at 10 16 55 AM
sulekhark commented 8 months ago

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):

issackjohn commented 8 months ago

Thank you all!