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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: froydnj, Assigned: bzbarsky)
References
Details
Attachments
(6 files, 2 obsolete files)
4.46 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
402 bytes,
text/html
|
Details | |
16.58 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
3.20 KB,
patch
|
Details | Diff | Splinter Review | |
2.38 KB,
patch
|
Details | Diff | Splinter Review | |
3.10 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
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 | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8356303 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8356304 -
Flags: review?(peterv)
Assignee | ||
Comment 6•10 years ago
|
||
Still need to measure performance impact.
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8356370 -
Flags: review?(peterv)
Assignee | ||
Comment 9•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Attachment #8356304 -
Attachment is obsolete: true
Attachment #8356304 -
Flags: review?(peterv)
Assignee | ||
Updated•10 years ago
|
Attachment #8356250 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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+
Reporter | ||
Comment 13•10 years ago
|
||
(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.
Reporter | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Comment 15•10 years ago
|
||
> 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?
Reporter | ||
Comment 16•10 years ago
|
||
(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?
Updated•10 years ago
|
Attachment #8356370 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b28eaf5bde9 https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a6bde76293
Flags: in-testsuite-
Target Milestone: --- → mozilla29
Comment 19•10 years ago
|
||
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
Assignee | ||
Comment 20•10 years ago
|
||
Nathan, do you have time to investigate that?
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 21•10 years ago
|
||
Also, possibly related: Regression: Mozilla-Inbound-Non-PGO - Dromaeo (CSS) - WINNT 6.1 (ix) - 2.32% decrease though maybt it's noise...
Assignee | ||
Comment 22•10 years ago
|
||
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...
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 26•10 years ago
|
||
Nathan was looking into what's going on here...
Assignee: bzbarsky → nfroyd
Assignee | ||
Updated•10 years ago
|
Target Milestone: mozilla29 → ---
Reporter | ||
Comment 27•10 years ago
|
||
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.
Reporter | ||
Updated•10 years ago
|
Assignee: nfroyd → bzbarsky
Comment 28•10 years ago
|
||
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
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•