Consider commoning up GetConstructorObjectHandle/GetProtoObjectHandle more

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

Adrian (cced) noted on irc about a month ago that GetConstructorObjectHandle for a binding does a bunch of stuff that is always the same, except for the constructor id used.  Same for GetProtoObjectHandle.

We should be able to common these up and save some codesize.
Looks like this saves ~320KB of codesize on Mac, out of a total libxul size of 182MB.
So _if_ I am measuring things right, I get the following for the overall size of "XUL" in an opt build:

Without patches: 182657024
With first cut:  182333440
With second cut: 182128640

So the inlining saves another 210KB or so?

I'll do some measurements on Linux too.
Assignee: nobody → bzbarsky
On Linux64, according to "size" we have:

Without patches: 119588271
With first cut:  119156127
With second cut: 119108007

So that first patch saves 430KB; the inlining saves another 48KB on top of that...  Hmm, I guess I should retry with --enable-release too.
Linux64, --enable-release:

Without patches: 100505504
With first cut:  100026472
With second cut: 100024968

So still a win there.
Attachment #8957438 - Flags: review?(peterv)
Since you're building m-c, even with --enable-release, you're still getting extra symbols table for profiling. If you changed something that affects the symbol table, you may want to compare stripped sizes.
(Relatedly, the main difference on Linux between --enable-release and without is ICF, I blows my mind that this makes a 19MB difference)
Comment on attachment 8957438 [details] [diff] [review]
Second cut, with more inlining but hence also more header inclusion

Review of attachment 8957438 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/BindingUtils.cpp
@@ +3830,5 @@
> +   * Calling fromMarkedLocation() is safe because protoAndIfaceCache is
> +   * traced by TraceProtoAndIfaceCache() and its contents are never
> +   * changed after they have been set.
> +   *
> +   * Calling address() avoids the read read barrier that does gray

"read read barrier"? :-)

@@ +3837,5 @@
> +
> +  const JS::Heap<JSObject*>& entrySlot =
> +    protoAndIfaceCache.EntrySlotMustExist(aSlotId);
> +  MOZ_ASSERT(JS::ObjectIsNotGray(entrySlot));
> +  return JS::Handle<JSObject*>::fromMarkedLocation(entrySlot.address());  

Stray whitespace.
Attachment #8957438 - Flags: review?(peterv) → review+
> If you changed something that affects the symbol table, you may want to compare stripped sizes.

I just tried "strip libxul.so" and the numbers I see are the same as in comment 6.  Note that the number I'm citing is the TEXT+DATA+BSS size according to "size", not the file size on disk.
> "read read barrier"? :-)

I just moved this code!  Fixed.

> Stray whitespace.

Fixed.

Comment 12

a year ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b750710adaa
Common up the Get*ObjectHandle methods in bindings.  r=peterv

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b750710adaa
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1444880
(In reply to Mike Hommey [:glandium] from comment #8)
> (Relatedly, the main difference on Linux between --enable-release and
> without is ICF, I blows my mind that this makes a 19MB difference)

Note that --enable-release also enables --gc-sections, which probably makes a significant difference on its own.
Blocks: 1446246
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.