Closed
Bug 965634
Opened 11 years ago
Closed 11 years ago
mozilla::dom::Register includes redundant nsString constructor/destructor calls
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: froydnj, Unassigned)
Details
(Whiteboard: [qa-])
Attachments
(1 file)
4.41 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Register is rather large, ~27K on x86-64. One of the things that makes it large is constructing an nsString for every call it does (mostly to RegisterDefineDOMInterface), and then destructing it after the call. This nsString is copied into a global hashtable.
Might it be worth it to pass in a raw char16_t*? We'd have to compute the length at runtime (or add another parameter to RegisterDefineDOMInterface, or something), but maybe that'd be worth it for cutting off a significant chunk of code.
![]() |
||
Comment 1•11 years ago
|
||
Passing a char16_t* and separate length argument makes total sense to me.
We could also try adding a template ctor to FakeDependentString that takes char16_t[N], or something, and using that here.
![]() |
Reporter | |
Comment 2•11 years ago
|
||
Here's a first cut at not constructing the nsStrings in Register() itself. The
template methods can optionally be compiled out of line if they are called more
than once.
On both x86-64 Linux and ARM Android, this cuts ~12K of space off the size of
Register(), even after accounting for the ~30 RegisterDefineDOMInterface template
helper methods that have been out-of-lined.
Trying to stuff everything into a single string would only win ~1.5K on ARM, not
sure if it's worth it. Would also complicate the code generator, as we'd have
to keep track of the lengths ourselves, and pass the lengths as arguments from
Register. Might come out to less 1.5K of win in the end.
Attachment #8368099 -
Flags: review?(bzbarsky)
![]() |
||
Comment 3•11 years ago
|
||
Comment on attachment 8368099 [details] [diff] [review]
move nsString construction out of mozilla::dom::Register for a codesize win
r=me, though I do wonder whether using FakeDependentString instead of nsDependentString would let you shave off more codesize.
Attachment #8368099 -
Flags: review?(bzbarsky) → review+
![]() |
Reporter | |
Comment 4•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #3)
> r=me, though I do wonder whether using FakeDependentString instead of
> nsDependentString would let you shave off more codesize.
I think it might help a little bit, but including BindingUtils.h in nsScriptNamespaceManager.h seems excessive. Why do the nsScriptNamespaceManager interfaces take nsAFlatString, rather than plain old nsAString? The compiler is unhappy about trying to pass FakeDependentString aka nsAString where nsAFlatString is desired.
![]() |
||
Comment 5•11 years ago
|
||
> Why do the nsScriptNamespaceManager interfaces take nsAFlatString
I don't know offhand....
![]() |
Reporter | |
Comment 6•11 years ago
|
||
Try informs me that using nsDependentString results in mass mochitest bustage (things like XMLHttpRequest not getting registered, apparently). However, since we're using this nice template, we can use nsLiteralString instead. It doesn't change the codesize win and it greens things up, so I'm just going to check the patch in with that change.
We can worry about FakeDependentString or similar in another bug.
Comment 7•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•11 years ago
|
Whiteboard: [qa-]
Assignee | ||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•