1357002 - Calling document.createElement() with a second arg that includes "is" should not have noticeable slowdown when custom elements feature is pref-ed off

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Edgar Chen [:edgar]]

Attachment: test_performance.html

This bug is filed from bug 1299363 comment #75.

> 1). pref-off, createElement without these changes, without any define() calls, with one arg
> 400.28
> 2). pref-off, createElement without these changes, without any define() calls, with a second arg that does not include "is". 
> 594.075
> 3). pref-off, createElement without these changes, without any define() calls, with a second arg that includes "is".
> 782.065

Above test result is based on,
- OS: Ubuntu 16.10
- Machine: Desktop, Intel(R) Core(TM) i7-3770 @ 3.40GHz, Memory 16G
- Build: pgo
- Rev: based on f9362554866b

Comment 1

[Edgar Chen [:edgar]]

I did the test again on Mac,

- OS: macOS 10.12.3
- Machine: Makbook Pro, 2.5 GHz Intel Core i7, Memory 16 GB
- Build: opt
- Rev: based on c697e756f738
- "dom.webcomponents.customelements.enabled" is false
- without any define() calls

-------------------------------+--------- 
1). with one arg               | 389.865 
-------------------------------+---------
2). with a second arg          |
    that does not include "is" | 436.335
-------------------------------+---------
3). with a second arg          |
    that includes "is"         | 715.735
-------------------------------+---------

Profile for 3: https://perfht.ml/2pHRxCQ

(6.0ms, 5.7%)
mozilla::dom::CustomElementRegistry::IsCustomElementEnabled(JSContext*, JSObject*)
  mozilla::Preferences::GetBool(char const*, bool*)
    PLDHashTable::Search(void const*)
       PLDHashTable::HashStringKey(void const*)

It seems it is worth to cache the ""dom.webcomponents.customelements.enabled" preference.

Comment 2

[Edgar Chen [:edgar]]

And SetupCustomElement() [1] don't need to be called if the custom element feature is pref-ed off.

[1] http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/dom/html/nsHTMLContentSink.cpp#267

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 1: Cache "dom.webcomponents.customelements.enabled" preference, v1

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 2: Don't call SetupCustomElement() if the custom element feature is pref-ed off, v1

Comment 5

[Edgar Chen [:edgar]]

Test result with the patches,

-------------------------------+--------- 
1). with one arg               | 405.304 
-------------------------------+---------
2). with a second arg          |
    that does not include "is" | 478.6515
-------------------------------+---------
3). with a second arg          |
    that includes "is"         | 562.565
-------------------------------+---------

Comment 6

[Edgar Chen [:edgar]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56cd1f5d8f3f51c681dee24cf539fd9c08155b56

Comment 7

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #6)
> Try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=56cd1f5d8f3f51c681dee24cf539fd9c08155b56

Debug build bustage: preferences in ContentPrefs.cpp should be sorted in alphabetical order.

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 1: Cache "dom.webcomponents.customelements.enabled" preference, v2, r=wchen

Address comment #7.

Comment 9

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44bf63ca8d47c1459267acd7912a6d87b100ced3&filter-tier=1&group_state=expanded

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac28a9153c8
Part 1: Cache "dom.webcomponents.customelements.enabled" preference; r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbd2216268bc
Part 2: Don't call SetupCustomElement() if the custom element feature is pref-ed off; r=wchen

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hrm.  Why does this preference need to be in ContentPrefs.cpp at all?

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ac28a9153c8
https://hg.mozilla.org/mozilla-central/rev/bbd2216268bc

Comment 13

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #11)
> Hrm.  Why does this preference need to be in ContentPrefs.cpp at all?

It is also used for some webidl condition [1], I afraid content process tries to access it too early.

[1] http://searchfox.org/mozilla-central/search?q=IsCustomElementEnabled&case=false&regexp=false&path=.webidl

Comment 14

[Boris Zbarsky [:bzbarsky]]

> It is also used for some webidl condition [1]

Yes, but those shouldn't get evaluated until we set up those objects.  Are we creating Window and Document objects before we've gotten the prefs from the parent process?

Comment 15

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #14)
> Yes, but those shouldn't get evaluated until we set up those objects.  Are
> we creating Window and Document objects before we've gotten the prefs from
> the parent process?

Sorry for doesn't make it clear.
The point of adding in ContentPrefs list here is that the pref value is cached in nsContentUtils and nsContentUtils::Init() happens quite early (before the pref init [1]). And there will be a short period that the cached value is not synced if it is not in ContentPrefs list. However, we won't run into such trouble since the CustomElementRegistry::IsCustomElementEnabled() accessing is happened after [1] per my test. We probably could consider caching the pref in other place (maybe in CustomElementRegistry) and then remove the pref from ContentPrefs list.

[1] http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/dom/ipc/ContentChild.cpp#970-974

Comment 16

[Boris Zbarsky [:bzbarsky]]

> The point of adding in ContentPrefs list here is that the pref value is cached in nsContentUtils

Right, but it's a bool var cache.  I was pretty sure those were exempted from the "accessing prefs too early" asserts, because we expect the consumer to handle the pref change when it comes in.

What I think we should do is:

1)  Remove this pref from the list.
2)  Add an assert that XRE_IsParentProcess() || mozilla::Preferences::InitPhase() == END_ALL_PREFS to
    the getter, to ensure that no one examines the value until after pref init.

It might be worth putting that assert condition into some helper, either in nsContentUtils.h or Preferences.h, because we should really do it in a bunch more places.