Closed Bug 956806 Opened 10 years ago Closed 10 years ago

generic getters/setters/methods for bindings take up a disproportionate amount of space

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: froydnj, Assigned: bzbarsky)

References

Details

Attachments

(6 files, 2 obsolete files)

For every WebIDL interface, we have a generic getter (and setter, if necessary) that handles unwrapping and dispatching to the appropriate codegen'd function.

I don't have a build in front of me to verify, but I remember these functions taking up ~80K combined.  (I can't remember offhand if that was on ARM or x86-64.)  From quick skimming of the generated functions, it looks like the only differences are the type of the unwrapped variable and the prototype ID.  The type of the unwrapped variable doesn't matter significantly because we immediately pass it into a |void*|-taking function pointer.

We ought to be able to commonify that generated code for a decent codesize win.
We can do this, with a few caveats:

1)  The getters actually need the interface name, for the exceptions they throw.  We
    could codegen a list of these and index into it with the proto id, I assume?
2)  Things that haveXPConnectImpls need the actual type to do the UnwrapArg<> thing, so
    we'd leave them out of this for now.
3)  We need to rejigger UnwrapObject to not be templated on the proto id.  Most simply,
    have the templated version just pass the id and depth to a non-templated version that
    takes those arguments, and also call the non-templated version here.
4)  We'll need to read the protoid and depth out of the jitinfo, which is presumably a
    slight bit slower than just hardcoding them.
Summary: generic getters/setters for bindings take up a disproportionate amount of space → generic getters/setters/methods for bindings take up a disproportionate amount of space
Oh, and for setters, there's an additional place where the interface name is used, like so:

  if (args.length() == 0) {
    return ThrowErrorMessage(cx, MSG_MISSING_ARGUMENTS, "Node attribute setter");
  }

but if we're OK with just making that rare case dynamically construct the string, it's not too bad to do either.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Still need to measure performance impact.
Attachment #8356370 - Flags: review?(peterv)
Without patches, I get times like so:

  12.82, 12.88, 13.01, 12.80, 12.82

with the patches, I get:

  13.04, 13.03, 13.01, 13.02, 13.10

So there's a small regression, but I don't think it's fatal.  And in non-microbenchmark code the better code locality might help, maybe.

With these patches, the only instances of generic* (as opposed to the lenient or cross-origin versions) left are for WorkerGlobalScope, DedicatedWorkerGlobalScope, SharedWorkerGlobalScope, Window, and EventTarget.  That's 13 methods, down from 881.  A stripped optimized libxul seems to be about 700KB smaller, on a 95MB baseline, but I'm not sure what to make of those numbers.

Nathan, want to try these patches out?
Attachment #8356304 - Attachment is obsolete: true
Attachment #8356304 - Flags: review?(peterv)
Attachment #8356250 - Attachment is obsolete: true
Oh, and we have 16 generic lenient things (all getters/setters; there are no lenient methods) and 3 cross-origin things so far (will pick up 3 more when we webidlify Location).  We could share these as well, but the codesize wins would be much smaller.  The main win might be being able to simplify or eliminate codegen code that outputs the generic methods right now.

Note: I considered codegenning the three generic things I ended up just hand-writing, but they're different enough from existing codegen stuff (due to the different unwrapping mechanics) that it gets pretty annoying.
Also, I aimed to minimize relocations in the new setup; I think there should be only two of them, but if I'm wrong please tell me!
Comment on attachment 8356303 [details] [diff] [review]
part 1.  Output a list of interface names in PrototypeList.cpp.

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

::: dom/bindings/Codegen.py
@@ +11445,5 @@
>  
> +        ifaceNamesWithProto = ['  "%s"' % d.interface.identifier.name
> +                               for d in descriptorsWithPrototype]
> +        traitsDecls.append(CGGeneric(
> +                declare=("extern const char* NamesOfInterfacesWithProtos[%d];\n\n" %

Should probably make this |const char* const|.
Attachment #8356303 - Flags: review?(peterv) → review+
(In reply to Boris Zbarsky [:bz] from comment #9)
> Nathan, want to try these patches out?

Before, building for ARM android:

[froydnj@cerebro build-android]$ readelf -s -W dom/bindings/*.o |grep generic |f 3 |addup
168196

After:

[froydnj@cerebro build-android]$ readelf -s -W dom/bindings/*.o |grep generic |f 3 |addup
7262

So that change saves ~150K of text.  I think it's worth it!

(In reply to Boris Zbarsky [:bz] from comment #11)
> Also, I aimed to minimize relocations in the new setup; I think there should
> be only two of them, but if I'm wrong please tell me!

The NamesOfInterfacesWithProtos table requires a relocation for every entry.  (The final address of each |char*| isn't known until runtime.)  That's pretty easy to fix, but deferring that to a followup bug is reasonable.
(In reply to Boris Zbarsky [:bz] from comment #10)
> Oh, and we have 16 generic lenient things (all getters/setters; there are no
> lenient methods) and 3 cross-origin things so far (will pick up 3 more when
> we webidlify Location).  We could share these as well, but the codesize wins
> would be much smaller.  The main win might be being able to simplify or
> eliminate codegen code that outputs the generic methods right now.

FWIW, the leneint* bits take up about 7K of text.  So there's some win in sharing them, but not a lot.  Simplifying codegen might be worth it, though.
> The NamesOfInterfacesWithProtos table requires a relocation for every entry.

That's pretty annoying.

I assume the right fix is to have a single big string with embedded nulls and then an array of indices into the string?  Do you want to do that, or should I?
(In reply to Boris Zbarsky [:bz] from comment #15)
> > The NamesOfInterfacesWithProtos table requires a relocation for every entry.
> 
> That's pretty annoying.

Yes, yes it is.

> I assume the right fix is to have a single big string with embedded nulls
> and then an array of indices into the string?  Do you want to do that, or
> should I?

I'll do it.  File a followup?
Filed bug 957175.
Blocks: 957175
Attachment #8356370 - Flags: review?(peterv) → review+
Sorry, but I had to back this out because mochitest-bc on OSX 10.6 debug runs became basically perma-fail after it landed, mostly hitting bug 925225.

https://hg.mozilla.org/mozilla-central/rev/0449f682dd31
Nathan, do you have time to investigate that?
Flags: needinfo?(nfroyd)
Also, possibly related:

Regression: Mozilla-Inbound-Non-PGO - Dromaeo (CSS) - WINNT 6.1 (ix) - 2.32% decrease

though maybt it's noise...
So I looked at the times for browser/base/content/test/general/browser_bug676619.js for the time these patches were in the tree.

They're pretty bimodal.  Some are in the 13-14s range.  Some are in the 31-34s range.  I saw nothing in between.  The >30s ones are the ones that are claimed as test timeouts (though with two different error messages).

It's possible that the test timeout is 30s and the tests are actually hanging sporadically, I guess...
Flags: needinfo?(nfroyd)
Nathan was looking into what's going on here...
Assignee: bzbarsky → nfroyd
Target Milestone: mozilla29 → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/28570c4d625e
https://hg.mozilla.org/integration/mozilla-inbound/rev/175ecf370564

The browser-chrome test in question has been disabled for too many intermittent failures.  Relanded.
Assignee: nfroyd → bzbarsky
https://hg.mozilla.org/mozilla-central/rev/28570c4d625e
https://hg.mozilla.org/mozilla-central/rev/175ecf370564
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: