Skip to content

Conversation

@nweldev
Copy link
Contributor

@nweldev nweldev commented Jul 9, 2020

attachDom can't be called on a tr element. Therefore, using an autonomous custom element is mandatory.
This leads to some issues with bootstrap, as it's expecting .table > tbody > tr.
In order to test this correctly, the list of elements shouldn't be a table, and bootstrap shouldn't be used.

See #764 (comment) & #753 (comment)

@ryansolid
Copy link
Contributor

You are hitting on why you never see shadow Dom in benchmarks. The need to mess with styles and change html structure basically makes it non-comparable. I hit this repeatedly when trying to make demos and benchmarks early on. As said in a previous thread i've built versions of this using native built-ins but never Shadow Dom for this reason.

Even with all custom styles and a non-restrictive HTML structure WebComponent version is creating more elements and doing more work. Trying to accommodate this across all benchmarks is a sizeable change.

@nweldev
Copy link
Contributor Author

nweldev commented Jul 9, 2020

You are hitting on why you never see shadow Dom in benchmarks.

IMO this project is pretty specific. Styles modularity can be achieved with or without a shadow dom. Here, the main issue is that this project is using a CSS library (bootstrap). And using table > tbody > tr isn't mandatory by itself. This wasn't built with this kind of problem in mind, and this is OK. But now it's too late for that.

Even with all custom styles and a non-restrictive HTML structure WebComponent version is creating more elements and doing more work. Trying to accommodate this across all benchmarks is a sizeable change.

Yep, this is exactly what I'm thinking too. Maybe a good fit for my fork, but not for this project.

I don't believe this PR should be merged (it's just an answer to #764 (comment)). Neither should have been #764 (which was a WIP) without discussing more about this.

@ryansolid
Copy link
Contributor

ryansolid commented Jul 9, 2020

IMO this project is pretty specific.

Other good or popular benchmarks:
https://github.com/localvoid/uibench
https://github.com/mathieuancelin/js-repaint-perfs
https://github.com/somebee/dom-reconciler-bench

Demos
https://github.com/gothinkster/realworld
https://github.com/tastejs/todomvc
Most hackernews implementations

The one thing they always have in common is non-modular CSS and more often than not for measuring purposes specific DOM structures. I do think the CSS can be worked through in a lot of the demos. Tables are particularly nasty and other benchmarks don't necessarily use those. And we've started to see WebComponent specific benchmarks(https://github.com/vogloblinsky/web-components-benchmark, or https://webcomponents.dev/blog/all-the-ways-to-make-a-web-component/). But it definitely highlights why you don't see even webcomponent libraries like Stencil or LitElement use the Shadow DOM in these demos.

I was talking to @trusktr recently about the desire to take a fork of JS Framework Benchmark in this direction, and I admitted I probably wouldn't have time but I think there would definitely be an interest there. Most webcomponent benchmarks are simplistic based on creation time or things like size. I will say from having participated in a few there are some standardization considerations. Some libraries using passed handlers versus DOM events to communicate upwards makes a pretty decent performance difference. But that's the whole thing. Maybe I've witnessed this, but this kind of data isn't out there for general consumption. I'm positive there is an interest.

@krausest
Copy link
Owner

krausest commented Jul 9, 2020

Since this benchmark includes rendering in the duration measurement it's very important to prevent optimizations for the styles for individual implementations.

I see two options:

  1. It should pretty easy to change the style for all benchmarks. Maybe it's time to drop the alternating row colors, create simple but still nice looking styles that are compatible with web components (but I'm not sure what's the best way to achieve that...) and see what impact it has on keyed vs. non-keyed.

  2. Allow additional css rules for webcomponent frameworks that create exactly the same look as bootstrap does. But I'm not sure how much those different styles influence the measurement. This might be dangerous.

What's your opinion?

@nweldev
Copy link
Contributor Author

nweldev commented Jul 9, 2020

@krausest It should pretty easy to change the style for all benchmarks

Big 👍 on this option.

@ryansolid I was talking to @trusktr recently about the desire to take a fork of JS Framework Benchmark in this direction, and I admitted I probably wouldn't have time but I think there would definitely be an interest there.

Me too, definitely. Like I said in #749 (comment), I'm/was thinking about coding:

  1. A project benchmarking just small VanillaJS approaches (e.g. innerHTML vs createElement) w/o anything else, in order to minimize the risk of unwanted side effects on performance. Of course, when talking about rendering, we need to get the whole parse+paint timing, like with this project. Maybe using puppeteer trace, or simply webdriver-ts.
  2. A tool allowing the automate web components' performance testing based on custom-elements-json files.

Unfortunately, I won't have time for that until the end of the year.

Actually, I spent too much time on this 😅 I gathered plenty enough information (thx to you 🙏), and I'll need to postpone all other discussions in order to focus on my actual work for some time. But I'll definitely get back to it ASAP.

@ryansolid
Copy link
Contributor

Yeah I think the first option seems reasonable for CSS. I think the more interesting thing is necessity of injecting the element in the table. @krausest does any of the checks look at the structure of the HTML. Correct me if I'm wrong @noelmace , but basically with webcomponents and shadow dom the structure is going to become:

<table>
  <tbody>
    <custom-row>
      #shadow-root
        <style />
        <tr>
          ...standard row stuff
        </tr>
    </custom-row>
  </tbody>
</table>

@nweldev
Copy link
Contributor Author

nweldev commented Jul 10, 2020

@ryansolid nope, that it. Using an element compatible with attachShadow would be better, but this would need too many changes.

@krausest
Copy link
Owner

So I created a very simple style which makes vanillajs and vanillajs-shadow look equal:
image
The results then look like that:

I know too little about web components. How often is the linked style evaluated when appending 1,000 web components?

@nweldev
Copy link
Contributor Author

nweldev commented Jul 21, 2020

How did you load the CSS in the shadow dom? Are you using a <link>? If not, The shadowed element seems very slow to render, compared to some other benchmarks I made in the past. But those other benchmarks were using very few CSS, so maybe this is only due to a heavy CSSOM to construct for each shadow, or because parsing the style takes a lot of time. We would need to investigate that in the Chrome DevTools.

Maybe @ryansolid will have some insights about that.

@krausest
Copy link
Owner

There‘s a style link cloned from the host document in the PR. See line 41 in Main.js:

shadow.appendChild(styleLink.cloneNode(true));

@ryansolid
Copy link
Contributor

That's interesting. There were a lot of things in spec or intended but I'm not sure how they landed in the browser. Chrome team at one point had talked about intention to optimize the processing of such style tags given that they could appear on the page multiple times but I'm not sure that ever happened. Honestly I'm unclear the fastest way anymore for Web Component styles. You should expect link tag to have cached the content so it should be able to pull already processed CSS.

@nweldev
Copy link
Contributor Author

nweldev commented Jul 22, 2020

A <link> will always lead to a fetch event. The only optimization a browser can do here is to use the cache. This is why I believe this is always slower than using a style.

I did some experiments recently to find a solution to pre-load an external CSS. First, to avoid any duplicate fetch, and second to minimize FOUC. Unfortunately, I was unable to find any good solution for that. This is why I recommend avoiding <link> in shadow dom as much as possible. Yet, I wasn't thinking about the use case we have here. Maybe the link could allow us to optimize rendering as parsing is already done? 🤷

@krausest could you share your code and benchmark another implementation using a <style> tag instead, with the same CSS, just to be sure?

@krausest
Copy link
Owner

krausest commented Jul 23, 2020

@noelmace I pushed the code to the branch vanillajs-shadow: https://github.com/krausest/js-framework-benchmark/tree/vanillajs-shadow
The vanillajs-shadow is your version and vanillajs-shadow-inline-style uses only inline styles. The reduced style is in css/slim.css which is copied to css/currentStyle.css to be used in (all correct) implementations.

I got the following results:

@krausest
Copy link
Owner

I'm closing this PR. I don't think we're getting it into master.

@krausest krausest closed this Jan 22, 2021
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.

4 participants