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

Perf: dom-change is expensive in FF with Shadow DOM polyfill #2389

Closed
nazar-pc opened this Issue Aug 30, 2015 · 14 comments

Comments

Projects
None yet
5 participants
@nazar-pc
Contributor

nazar-pc commented Aug 30, 2015

I have following element structure:

<dom-module id="cs-system-admin-components-modules-list">
    <template>
        <cs-table center list with-header>
            <cs-table-row>
                <cs-table-cell>[[L.module_name]]</cs-table-cell>
                <cs-table-cell>[[L.state]]</cs-table-cell>
                <cs-table-cell>[[L.action]]</cs-table-cell>
            </cs-table-row>
            <template as="module" is="dom-repeat" items="[[modules]]">
                <cs-table-row class$="[[module.class]]">
                    <cs-table-cell>
                        <template if="[[module.administration]]" is="dom-if">
                            <a href="[[concat('/admin/', module.name)]]" tooltip="[[module.info]]">
                                <span>[[module.name_localized]]</span>
                                <cs-tooltip></cs-tooltip>
                            </a>
                        </template>
                        <template if="[[!module.administration]]" is="dom-if">
                          <span tooltip="[[module.info]]">
                            <span>[[module.name_localized]]</span>
                            <cs-tooltip></cs-tooltip>
                          </span>
                        </template>
                    </cs-table-cell>
                    <cs-table-cell>
                        <cs-icon icon="[[module.icon]]" tooltip="[[module.icon_text]]"></cs-icon>
                        <template if="[[module.api]]" is="dom-if">
                            <template if="[[!module.api.type]]" is="dom-if">
                                <cs-icon icon="link" tooltip="[[L.api_exists]]"></cs-icon>
                            </template>
                            <template if="[[module.api.type]]" is="dom-if">
                                <button icon="link" is="cs-button" tooltip="[[concat(L.api_exists, '<br>', L.click_to_view_details)]]" type="button"></button>
                                <section content="[[module.api.content]]" is="cs-section-modal"></section>
                            </template>
                        </template>
                        <template if="[[module.readme]]" is="dom-if">
                            <button icon="book" is="cs-button" tooltip="[[concat(L.information_about_module, '<br>', L.click_to_view_details)]]" type="button"></button>
                            <section content="[[module.readme.content]]" is="cs-section-modal"></section>
                        </template>
                        <template if="[[module.license]]" is="dom-if">
                            <button icon="legal" is="cs-button" tooltip="[[concat(L.license, '<br>', L.click_to_view_details)]]" type="button"></button>
                            <section content="[[module.license.content]]" is="cs-section-modal"></section>
                        </template>
                    </cs-table-cell>
                    <cs-table-cell left>
                        <template if="[[module.can_be_set_as_default]]" is="dom-if">
                            <a href="[[concat('/admin/System/components/modules/default_module/', module.name)]]" icon="home" is="cs-link-button" tooltip="[[L.make_default_module]]"></a>
                        </template>
                        <template if="[[module.db_settings]]" is="dom-if">
                            <a href="[[concat('/admin/System/components/modules/db/', module.name)]]" icon="database" is="cs-link-button" tooltip="[[L.databases]]"></a>
                        </template>
                        <template if="[[module.storage_settings]]" is="dom-if">
                            <a href="[[concat('/admin/System/components/modules/storage/', module.name)]]" icon="hdd-o" is="cs-link-button" tooltip="[[L.storages]]"></a>
                        </template>
                        <template if="[[module.administration]]" is="dom-if">
                            <a href="[[concat('/admin/', module.name)]]" icon="sliders" is="cs-link-button" tooltip="[[L.module_admin_page]]"></a>
                        </template>
                        <template if="[[equal(module.active, 1)]]" is="dom-if">
                            <a href="[[concat('/admin/System/components/modules/disable/', module.name)]]" icon="minus" is="cs-link-button" tooltip="[[L.disable]]"></a>
                        </template>
                        <template if="[[equal(module.active, 0)]]" is="dom-if">
                            <a force-compact href="[[concat('/admin/System/components/modules/enable/', module.name)]]" icon="check" is="cs-link-button" tooltip="[[L.enable]]">[[L.enable]]</a>
                        </template>
                        <template if="[[!equal(module.active, -1)]]" is="dom-if">
                            <a href="[[concat('/admin/System/components/modules/uninstall/', module.name)]]" icon="trash" is="cs-link-button" tooltip="[[L.uninstall]]"></a>
                        </template>
                        <template if="[[equal(module.active, -1)]]" is="dom-if">
                            <a force-compact href="[[concat('/admin/System/components/modules/install/', module.name)]]" icon="download" is="cs-link-button" tooltip="[[L.install]]">[[L.install]]</a>
                        </template>
                    </cs-table-cell>
                </cs-table-row>
            </template>
        </cs-table>
    </template>
    <script src="script.js"></script>
</dom-module>

Custom elements themselves are fast, I've optimized them as much as I can, cs-table* are not registered elements, just used for styling.

But such nesting, namely dom-bind > dom-if > dom-if causes a lot of dom-change events, which with Shadow DOM polyfill are just way too expensive and takes enormous amount of time even for 20 elements in [[modules]].

I'd suggest to add possibility to optionally disable firing this event. Or (with some bc break), do not fire it by default, but only when it is actually needed and explicitly specified.

@ebidel

This comment has been minimized.

Member

ebidel commented Aug 30, 2015

To be clear, are you using the shadow dom polyfill or webcomponents-lite.js (shady dom)? The latter will be faster, especially in FF.

You should also consider not using dom-if for trivial amounts of markup. Instead, use hidden$:

<a href="..." hidden$="[[!module.administration]]"></a>
@nazar-pc

This comment has been minimized.

Contributor

nazar-pc commented Aug 30, 2015

I do use full polyfill with full Shadow DOM emulation. Polymer.dom(..) seems too ugly for me and eventually everything will move to Shadow DOM.
Also I've migrated many components from Polymer 0.5, so I stick to Shadow DOM currently.

I'm trying to use hidden$= as often as I can, but it is not always convenient and doesn't solve root issue. Just avoiding firing dom-change can save a lot of resources, I guess under Shady DOM it will have measurable performace improvement as well.

@ebidel

This comment has been minimized.

Member

ebidel commented Aug 30, 2015

What makes you think firing the event is the bottleneck? It would be interesting to see what happens to the perf when moving to webcomponents-lite.js. Just as an experiment. For your app, it may be that you don't even need to touch Polymer.dom()

@nazar-pc

This comment has been minimized.

Contributor

nazar-pc commented Aug 30, 2015

I'm touching this.shadowRoot now, so I''ll need separation for light/local trees.

About dom-change - I've being tracing and comparing performance using Performance tab in FF Dev Tools.
Here is screenshot, huge yellow bar is load event and very long chain of dom-chage events without any handlers below. Each call is fast - few ms, but their amount is enormous (just look at chain slope).

Also I've edited source code commenting dom-change firing for dom-if and dom-repeat and confirmed that everithing becomes much better.
Together with hacking on #2387 issue I've got significant reduction of page rendering.

2015-08-30 19-03-06

@ebidel

This comment has been minimized.

Member

ebidel commented Aug 30, 2015

I still think it's worth trying not to use so many dom-if. That would greatly reduce the number of events fired. BTW, do you need to handle the event? If not, drop the event on the floor and don't create a handler for it.

performance of shady dom in polyfill'd browsers is one of the key differences from 0.5's full shadow dom polyfill. Most (if not all) of our tests are targeting it rather than the full polyfill these days and we see it as the path forward for Polymer apps until Shadow DOM is native everywhere. If you're worried about using Polymer.dom(), check out https://github.com/PolymerLabs/polymer-experiments/blob/master/patch-dom.html. That patches the DOM methods tososhady behaves like shadow.

So in shady dom, your call would be: Polymer.dom(this.root).

@nazar-pc

This comment has been minimized.

Contributor

nazar-pc commented Aug 30, 2015

BTW, do you need to handle the event? If not, drop the event on the floor and don't create a handler for it.

I don't need to handle it, what you mean by drop the event on the floor? There are no handlers for it, but that doesn't mean event is not fired.

I think to finally try Shady DOM with that experiment and compare how it affects dom-change firing.

@nazar-pc

This comment has been minimized.

Contributor

nazar-pc commented Aug 30, 2015

Tried experiment - causes TypeError: Illegal invocation at Object.Polymer.DomApi.ctor.patch, both version from mentioned repository and from this.

WebComponents-light.js does work faster (and visually even ~70% faster when dropping dom-change calls from Polymer code), but it causes styling issues in Chromium (minor) and completely destroys styling in Firefox (my CSS files are Shadow DOM aware already and thus without proper parsing they'll not work at all).

So, this is not a solution for me. I'll reduce number of dom-if usages, but still, option like no-dom-change would be useful.

@miztroh

This comment has been minimized.

miztroh commented Aug 30, 2015

When you say your CSS files are Shadow DOM aware, you're not talking about
using ::shadow and /deep/ are you? Those have been deprecated and will
eventually not work in any browsers.
On Aug 30, 2015 1:19 PM, "Nazar Mokrynskyi" notifications@github.com
wrote:

Tried experiment - causes TypeError: Illegal invocation at
Object.Polymer.DomApi.ctor.patch, both version from mentioned repository
and from this.

WebComponents-light.js does work faster (and visually even ~70% faster
when dropping dom-change calls from Polymer code), but it causes styling
issues in Chromium (minor) and completely destroys styling in Firefox (my
CSS files are Shadow DOM aware already and thus without proper parsing
they'll not work at all).

So, this is not a solution for me. I'll reduce number of dom-if usages,
but still, option like no-dom-change would be useful.


Reply to this email directly or view it on GitHub
#2389 (comment).

@nazar-pc

This comment has been minimized.

Contributor

nazar-pc commented Aug 31, 2015

@miztroh, yes, you are right, but when they'll be removed there will be (hopefully) some improvement in Shadow DOM awareness in third-party components. I've patched many libraries to work fine under Shadow DOM. In terms of JS funtionality authors often accept pull requests, but it is not so simple with CSS part. I've being fighting, for instance, with UIkit, but since they do not want to actively accept patches I've ended up rewriting all used widgets from scratch. New components work, they work even better, they have data bindings, but it takes so much sime to do that, because there are not that much already written components available.

Another significant feature I'm lacking is #2319, which also currently can be implemented using /deep/. There are simply so many thing yet to do, can't afford to get rid of /deep/ and ::shadow now.

Crossing Shadow DOM is essential for me during transition phase.

@miztroh

This comment has been minimized.

miztroh commented Aug 31, 2015

@nazar-pc:
For global styles, you can just do it using the new shared style recommendations and a <style is="custom-style"> tag inside head that includes the shared style dom-module.

#2229
#2188 (comment)

My shared style rules follow this pattern. This allows the rules to cover the local dom of a custom element as well as any distributed children.

a,
::content a {
    color: blue;
}

paper-button,
::content paper-button {
    color: red;
}

I strongly advise against depending on /deep/ and ::shadow at all. It'll save you a bunch of headache in the future.

@nazar-pc

This comment has been minimized.

Contributor

nazar-pc commented Aug 31, 2015

@miztroh I know about that, but:

  1. No SCSS
  2. Inline CSS (instead refering CSS file)
  3. Manual importing everywhere is not really nice
  4. Takes more manual work with third-party styles
  5. You can't forseen all necessary references in custom element to apply additional styles later

Now compare all that with 3 lines of SCSS:

html /deep/ {
    @import "fotorama";
}

It just wraps source CSS.

Eventually this is off-topic. You can find me in many places if you want to continue this discussion.

@nazar-pc

This comment has been minimized.

Contributor

nazar-pc commented Sep 4, 2015

Rewrote in such way that only one dom-if is used, and hidden$= in other places - performance didn't really become better (subjectively it seems to be even worse).
While dom-change definitely have significant impact, there is something with bindings propagation, because I have them quite a lot here.

I'll investigate deeper when have more time, but I suspect one-time bindings with restamp for dom-repeat would be really handy here instead of current approach.

@ebidel

This comment has been minimized.

Member

ebidel commented Sep 4, 2015

Maybe @sjmiles @kevinpschaaf @sorvell can offer some suggestions or see if there's low hanging fruit.

@nazar-pc

This comment has been minimized.

Contributor

nazar-pc commented Sep 4, 2015

Here is how it looks with modifications:

<dom-module id="cs-system-admin-components-modules-list">
    <link href="style.css" rel="import" type="css">
    <template>
        <style include="advanced-styles"></style>
        <table center class="cs-table" list>
            <tr>
                <th>[[L.module_name]]</th>
                <th>[[L.state]]</th>
                <th>[[L.action]]</th>
            </tr>
            <template as="module" is="dom-repeat" items="[[modules]]">
                <tr class$="[[module.class]]">
                    <td>
                        <a hidden$="[[!module.administration]]" href="[[concat('/admin/', module.name)]]" tooltip="[[module.info]]">
                            <span>[[module.name_localized]]</span>
                            <cs-tooltip></cs-tooltip>
                        </a>
                        <span hidden$="[[module.administration]]" tooltip="[[module.info]]">
                            <span>[[module.name_localized]]</span>
                            <cs-tooltip></cs-tooltip>
                        </span>
                    </td>
                    <td>
                        <cs-icon icon="[[module.icon]]" tooltip="[[module.icon_text]]"></cs-icon>
                        <template if="[[module.api]]" is="dom-if">
                            <cs-icon hidden$="[[module.api.type]]" icon="link" tooltip="[[L.api_exists]]"></cs-icon>
                            <button hidden$="[[!module.api.type]]" icon="link" is="cs-button" tooltip="[[concat(L.api_exists, '<br>', L.click_to_view_details)]]" type="button"></button>
                            <section content="[[module.api.content]]" is="cs-section-modal"></section>
                        </template>
                        <button hidden$="[[!module.readme]]" icon="book" is="cs-button" tooltip="[[concat(L.information_about_module, '<br>', L.click_to_view_details)]]" type="button"></button>
                        <section content="[[module.readme.content]]" is="cs-section-modal"></section>
                        <button hidden$="[[!module.license]]" icon="legal" is="cs-button" tooltip="[[concat(L.license, '<br>', L.click_to_view_details)]]" type="button"></button>
                        <section content="[[module.license.content]]" is="cs-section-modal"></section>
                    </td>
                    <td left>
                        <a hidden$="[[!module.can_be_set_as_default]]" href="[[concat('/admin/System/components/modules/default_module/', module.name)]]" icon="home" is="cs-link-button" tooltip="[[L.make_default_module]]"></a>
                        <a hidden$="[[!module.db_settings]]" href="[[concat('/admin/System/components/modules/db/', module.name)]]" icon="database" is="cs-link-button" tooltip="[[L.databases]]"></a>
                        <a hidden$="[[!module.storage_settings]]" href="[[concat('/admin/System/components/modules/storage/', module.name)]]" icon="hdd-o" is="cs-link-button" tooltip="[[L.storages]]"></a>
                        <a hidden$="[[!module.administration]]" href="[[concat('/admin/', module.name)]]" icon="sliders" is="cs-link-button" tooltip="[[L.module_admin_page]]"></a>
                        <a hidden$="[[!equal(module.active, 1)]]" href="[[concat('/admin/System/components/modules/disable/', module.name)]]" icon="minus" is="cs-link-button" tooltip="[[L.disable]]"></a>
                        <a force-compact hidden$="[[!equal(module.active, 0)]]" href="[[concat('/admin/System/components/modules/enable/', module.name)]]" icon="check" is="cs-link-button" tooltip="[[L.enable]]">[[L.enable]]</a>
                        <a hidden$="[[equal(module.active, -1)]]" href="[[concat('/admin/System/components/modules/uninstall/', module.name)]]" icon="trash" is="cs-link-button" tooltip="[[L.uninstall]]"></a>
                        <a force-compact hidden$="[[!equal(module.active, -1)]]" href="[[concat('/admin/System/components/modules/install/', module.name)]]" icon="download" is="cs-link-button" tooltip="[[L.install]]">[[L.install]]</a>
                    </td>
                </tr>
            </template>
        </table>
    </template>
    <script src="script.js"></script>
</dom-module>

Firefox hangs for some seconds, for Chromium it takes about 2 seconds to render page with 20 items.

@sorvell sorvell assigned kevinpschaaf and unassigned sorvell Nov 11, 2015

@sorvell sorvell added the p1 label Nov 11, 2015

@nazar-pc nazar-pc closed this Aug 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment