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

Add Web Components and LIT versions of Complex-DOM TodoMVC #285

Merged

Conversation

lpardosixtosMs
Copy link
Contributor

@lpardosixtosMs lpardosixtosMs commented Jul 13, 2023

This PR adds the Web Components and LIT versions of the Complex-DOM todoMVC workload.

Changes to the complex DOM generator:

  • Added constructable stylesheets under the utils folder to be included by the LIT and WC complex todoMVC.

Changes to the standalone versions.

  • Added the show-priority class name to the todo-list elements.
  • Added the data-priority attribute to the todo-item elements.
  • The list and list items components now query if the global variables window.extraTodoListCssToAdopt and window.extraCssToAdopt are populated and adopt the stylesheets stored in them. The variables should not be defined on the standalone version, and they are populated in the complex version from another script.

This PR DOESN'T modify which workloads run in the benchmark. That will be done in a follow up PR.

Workload design: https://docs.google.com/document/d/1QoO1VzFf3PZY-lsHdHFKGX6m1p393iEqjHQOzmU0NUI/edit?usp=sharing

Hosted version: https://lpardosixtosMs.github.io/SpeedometerWebComponents/?suite=TodoMVC-WebComponents,TodoMVC-WebComponents-Complex,TodoMVC-LIT,TodoMVC-LIT-Complex-DOM

@lpardosixtosMs lpardosixtosMs marked this pull request as draft July 13, 2023 23:54
@lpardosixtosMs lpardosixtosMs marked this pull request as ready for review July 14, 2023 00:15
@lpardosixtosMs
Copy link
Contributor Author

@flashdesignory Please let me know if this approach looks good, this should work for the LIT implementation too, which we'll clean up tomorrow.

.eslintignore Outdated Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
@flashdesignory
Copy link
Contributor

@lpardosixtosMs - can you also add scores standalone / complex?

@flashdesignory
Copy link
Contributor

index logic breaks:
Screenshot 2023-07-13 at 8 15 14 PM

resources/tests.mjs Outdated Show resolved Hide resolved
@lpardosixtosMs
Copy link
Contributor Author

@lpardosixtosMs - can you also add scores standalone / complex?

Yes, I will post them here once we get the LIT implementation ready.

@lpardosixtosMs lpardosixtosMs changed the title JavaScript-Web-Components Complex DOM TodoMVC Add Web Components and LIT versions of Complex-DOM TodoMVC Jul 14, 2023
@lpardosixtosMs
Copy link
Contributor Author

@lpardosixtosMs - can you also add scores standalone / complex?

@flashdesignory here are the numbers:

OS: macos 13.4.1 arm64. Device: MacBookPro17,1. CPU: Apple M1 8 cores.

TodoMVC-Lit-Complex-DOM/total

  • Firefox: 30.24 ± 1.0%
  • Chrome: 23.81 ± 0.94%
  • Safari: 85.5 ± 1.2%

TodoMVC-Lit/total

  • Firefox: 23.07 ± 3.9%
  • Chrome: 18.06 ± 0.67%
  • Safari: 21.09 ± 2.2%

TodoMVC-WebComponents-Complex/total

  • Firefox: 37.32 ± 1.8%
  • Chrome: 26.912 ± 0.22%
  • Safari 69.92 ± 1.1%

TodoMVC-WebComponents/total

  • Firefox: 28.17 ± 0.79%
  • Chrome: 23.96 ± 1.7%
  • Safari: 22.55 ± 2.5%

Sanity check: Numbers before this PR:

TodoMVC-Lit/total

  • Firefox: 23.18 ± 3.7%
  • Chrome: 17.44 ± 2.7%
  • Safari: 20.91 ± 1.6%

TodoMVC-WebComponents/total

  • Firefox: 29.25 ± 1.5%
  • Chrome: 24.13 ± 1.9%
  • Safari: 23.70 ± 3.4%

@issackjohn issackjohn self-requested a review July 14, 2023 22:37
resources/tests.mjs Outdated Show resolved Hide resolved
@lpardosixtosMs
Copy link
Contributor Author

@rniwa @julienw I'm not sure if this qualifies as trivial change. Do you want to take look?

@flashdesignory
Copy link
Contributor

@rniwa @julienw I'm not sure if this qualifies as trivial change. Do you want to take look?

It does change scores and should get two approvals.
Also @bgrins might be able to take a look since @julienw might not be able to get to this pr?

@benjamind
Copy link

Really great to see web components and shadowdom based tests going in here!

We (Adobe) have much more complex scenarios and patterns of web components and shadowdom usage than even these complex benchmarks, and we see similar performance metrics across the browsers as these new numbers are indicating.

Definitely worth having these more complex cases as some of our web apps have upwards of 8k+ nodes on the page at a time, as well as very deeply nested ShadowDOM trees, as a result we see performance degradation in some browsers in these cases, so its good to get these benchmarked so they can be improved upon.

@issackjohn
Copy link
Contributor

issackjohn commented Aug 10, 2023

Not for this PR but I thought this would be a relevant place to discuss the new PR I'm working on.

@flashdesignory the Lit version of the TodoMVC sets the "completed" classname to the li like the rest of the frameworks

todomvc-lit

override render() {
        const itemClassList = {
            todo: true,
            completed: this.completed ?? false,
            editing: this.isEditing,
        };
        return html`
            <li class="${classMap(itemClassList)}">

but the javascript-web-components TodoMVC uses an attribute "completed" in the custom component "todo-item" instead.

toggleItem() {
        // The todo-list checks the "completed" attribute to filter based on route
        // (therefore the completed state needs to already be updated before the check)
        this.setAttribute("completed", this.toggleInput.checked);

        this.dispatchEvent(
            new CustomEvent("toggle-item", {
                detail: { id: this.id, completed: this.toggleInput.checked },
                bubbles: true,
            })
        );
    }

Is it possible to make either of these use the same approach as the other, this would help with the complex versions being able to share handcrafted CSS. Is this possible? Or what would you suggest?

@flashdesignory
Copy link
Contributor

Is it possible to make either of these use the same approach as the other, this would help with the complex versions being able to share handcrafted CSS. Is this possible? Or what would you suggest?

The web component version is the only workload so far that is built according to my proposed css / a11y changes and therefore the structure is different from the other workloads. We are postponing making the same changes to the other todomvc apps and saving it for the next version (S4).

If you want to make a change in the web component version, please add comments so that we can revert the changes at a later point. Also, I would prefer not to touch the /todomvc-css folder, since that's set up for the new versions already.

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.

Just a few early comments. I think this will need to be updated after #291 anyway, right?

@issackjohn
Copy link
Contributor

Just a few early comments. I think this will need to be updated after #291 anyway, right?

Thank you! and yes that's right.

@issackjohn
Copy link
Contributor

Is it possible to make either of these use the same approach as the other, this would help with the complex versions being able to share handcrafted CSS. Is this possible? Or what would you suggest?

The web component version is the only workload so far that is built according to my proposed css / a11y changes and therefore the structure is different from the other workloads. We are postponing making the same changes to the other todomvc apps and saving it for the next version (S4).

If you want to make a change in the web component version, please add comments so that we can revert the changes at a later point. Also, I would prefer not to touch the /todomvc-css folder, since that's set up for the new versions already.

Okay that makes sense. Thanks!

issackjohn and others added 6 commits August 17, 2023 11:44
Add Web Components todoMVC complex DOM implementation.

Co-authored-by: Luis Fernando Pardo Sixtos <lpardosixtos@microsoft.com>
Add complex DOM version of LIT.

Co-authored-by: Luis Fernando Pardo Sixtos <lpardosixtos@microsoft.com>
@issackjohn issackjohn force-pushed the lpardosixtos/ComplexDOMWebComponents branch from 9d415c6 to f82c856 Compare August 17, 2023 19:08
@lpardosixtosMs
Copy link
Contributor Author

@flashdesignory @julienw. I updated the workload to work with handcrafted rules instead of a random generator. Can you take another look? I also updated the description and the workload design document. Here are the most recent perf numbers:

Before:

OS: macos 13.4.1 arm64. Device: MacBookPro17,1. CPU: Apple M1 8 cores.

TodoMVC-Lit-Complex-DOM/total
Firefox: 38.2 ± 3.1%
Chrome: 29.65 ± 0.65%
Safari: 62.76 ± 1.4%

TodoMVC-Lit/total
Firefox: 20.49 ± 1.4%
Chrome: 16.98 ± 0.61%
Safari: 21.16 ± 2.8%

TodoMVC-WebComponents-Complex/total
Firefox: 45.52 ± 1.4%
Chrome: 27.73 ± 0.48%
Safari 70.23 ± 1.1%

TodoMVC-WebComponents/total
Firefox: 28.13 ± 1.9%
Chrome: 23.79 ± 0.45%
Safari: 23.39 ± 3.1%

Sanity check. Numbers before this PR:

TodoMVC-Lit/total
Firefox: 21.64 ± 3.0%
Chrome: 17.280 ± 0.29%
Safari: 21.30 ± 3.0%

TodoMVC-WebComponents/total
Firefox: 29.87 ± 0.46%
Chrome: 23.478 ± 0.33%
Safari: 23.79 ± 1.6%

@flashdesignory
Copy link
Contributor

@lpardosixtosMs @issackjohn - is the requested change from @issackjohn completed and resolved?

@lpardosixtosMs
Copy link
Contributor Author

@lpardosixtosMs @issackjohn - is the requested change from @issackjohn completed and resolved?

Yes, but he's been out of office, that's why it is still there.

Copy link
Contributor

@flashdesignory flashdesignory left a comment

Choose a reason for hiding this comment

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

some minor cleanup nits

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, tell me what you think!

@lpardosixtosMs
Copy link
Contributor Author

Addressed the feedback, Thanks!

@flashdesignory @julienw I realized that adding the extra style sheet in the connectedCallback didn't make sense and could cause extra overhead when connecting the todo items. I refactored it so it is added either in the constructor (web components) or as part of the styles variable override (LIT).

@rniwa this change doesn't enable any test by default, do you want to take a look before merging?

Copy link
Member

@rniwa rniwa left a comment

Choose a reason for hiding this comment

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

Looks sane to me.

@flashdesignory
Copy link
Contributor

Addressed the feedback, Thanks!

@flashdesignory @julienw I realized that adding the extra style sheet in the connectedCallback didn't make sense and could cause extra overhead when connecting the todo items. I refactored it so it is added either in the constructor (web components) or as part of the styles variable override (LIT).

@lpardosixtosMs - these changes are only related to complex dom styles, correct?

@lpardosixtosMs
Copy link
Contributor Author

Addressed the feedback, Thanks!
@flashdesignory @julienw I realized that adding the extra style sheet in the connectedCallback didn't make sense and could cause extra overhead when connecting the todo items. I refactored it so it is added either in the constructor (web components) or as part of the styles variable override (LIT).

@lpardosixtosMs - these changes are only related to complex dom styles, correct?

@flashdesignory correct. In the standalone case the extra stylesheet is not added (Web components) or an empty style entry is added (LIT).

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

@lpardosixtosMs lpardosixtosMs merged commit 679cb52 into WebKit:main Sep 6, 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.

None yet

6 participants