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

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: dom-ce-m2)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a month ago
Created attachment 8858756 [details]
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
(Assignee)

Comment 1

a month ago
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.
(Assignee)

Comment 2

a month ago
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
(Assignee)

Comment 3

a month ago
Created attachment 8858766 [details] [diff] [review]
Part 1: Cache "dom.webcomponents.customelements.enabled" preference, v1
(Assignee)

Comment 4

a month ago
Created attachment 8858767 [details] [diff] [review]
Part 2: Don't call SetupCustomElement() if the custom element feature is pref-ed off, v1
(Assignee)

Comment 5

a month ago
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
-------------------------------+---------
(Assignee)

Updated

a month ago
Attachment #8858766 - Flags: review?(wchen)
(Assignee)

Updated

a month ago
Attachment #8858767 - Flags: review?(wchen)

Updated

a month ago
Attachment #8858767 - Flags: review?(wchen) → review+

Updated

a month ago
Attachment #8858766 - Flags: review?(wchen) → review+
(Assignee)

Comment 6

a month ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56cd1f5d8f3f51c681dee24cf539fd9c08155b56
(Assignee)

Updated

a month ago
Whiteboard: dom-ce-m2
(Assignee)

Comment 7

a month ago
(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.
(Assignee)

Comment 8

a month ago
Created attachment 8859940 [details] [diff] [review]
Part 1: Cache "dom.webcomponents.customelements.enabled" preference, v2, r=wchen

Address comment #7.
Attachment #8859940 - Flags: review+
(Assignee)

Updated

a month ago
Attachment #8858766 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Comment 10

a month ago
Pushed by echen@mozilla.com:
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
Hrm.  Why does this preference need to be in ContentPrefs.cpp at all?
Flags: needinfo?(echen)
https://hg.mozilla.org/mozilla-central/rev/0ac28a9153c8
https://hg.mozilla.org/mozilla-central/rev/bbd2216268bc
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 13

a month ago
(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
Flags: needinfo?(echen)
> 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?
(Assignee)

Comment 15

a month ago
(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
> 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.
You need to log in before you can comment on or make changes to this bug.