Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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
3 months ago
3 months 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)

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
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.
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
Created attachment 8858766 [details] [diff] [review]
Part 1: Cache "dom.webcomponents.customelements.enabled" preference, v1
Created attachment 8858767 [details] [diff] [review]
Part 2: Don't call SetupCustomElement() if the custom element feature is pref-ed off, v1
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
-------------------------------+---------
Attachment #8858766 - Flags: review?(wchen)
Attachment #8858767 - Flags: review?(wchen)

Updated

3 months ago
Attachment #8858767 - Flags: review?(wchen) → review+

Updated

3 months ago
Attachment #8858766 - Flags: review?(wchen) → review+
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56cd1f5d8f3f51c681dee24cf539fd9c08155b56
Whiteboard: dom-ce-m2
(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.
Created attachment 8859940 [details] [diff] [review]
Part 1: Cache "dom.webcomponents.customelements.enabled" preference, v2, r=wchen

Address comment #7.
Attachment #8859940 - Flags: review+
Attachment #8858766 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44bf63ca8d47c1459267acd7912a6d87b100ced3&filter-tier=1&group_state=expanded

Comment 10

3 months 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

Comment 11

3 months ago
Hrm.  Why does this preference need to be in ContentPrefs.cpp at all?
Flags: needinfo?(echen)

Comment 12

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0ac28a9153c8
https://hg.mozilla.org/mozilla-central/rev/bbd2216268bc
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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)

Comment 14

3 months ago
> 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?
(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

3 months ago
> 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.