Consider removing NSCAP_FEATURE_USE_BASE
Categories
(Core :: XPCOM, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox115 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(3 files)
This forces us to have hacks in the stylo bindings to deal with debug and release builds having different used template parameters, and seems that it could be a win if we plan to make use of `final` refcounting methods in stuff like nsIAtom, which the compiler should be able to devirtualize. Nathan told me that this feature existed mainly because of codesize reasons, so I did two try runs, one with it enabled, and one with it disabled, and here are the results: Feature enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbc5476d904d72018eeb71be09fd73bdbe5728d4 ======== Size of libxul.so: Linux PGO: 100218064 bytes Linux x64 PGO: 104317992 bytes Android 4.0 API15+: 21845876 bytes Android 4.2 x86: 21846756 bytes Size of target zip: Windows 2012 PGO: 52252651 bytes Windows 2012 x64 PGO: TBD, build failed intermittently Disabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=100b9856c6aee244feff223c61c2d6fe7250e8ab ========= Size of libxul.so: Linux PGO: 100498568 bytes Linux x64 PGO: 104541808 bytes Android 4.0 API15+: 21881812 bytes Android 4.2 x86: 21946704 bytes Size of target zip: Windows 2012 PGO: 52287201 bytes Windows 2012 x64 PGO: 56194506 bytes
Assignee | ||
Comment 1•7 years ago
|
||
Looks like the biggest regression is ~220kb in size in linux. It doesn't look like a lot to me, but I'm not an expert in this kind of stuff. Nathan, do you know if that's an acceptable price to pay?
Comment 2•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1) > Looks like the biggest regression is ~220kb in size in linux. It doesn't > look like a lot to me, but I'm not an expert in this kind of stuff. Nathan, > do you know if that's an acceptable price to pay? 220KB is quite a lot... Can you measure instead the real binary sizes of libxul on different platforms? e.g. for Linux, for my running firefox, I get: froydnj@hawkeye:~/src/gecko-dev.git$ ls -l ~/firefox/libxul.so -rwxr-xr-x 1 froydnj froydnj 104324592 Apr 28 13:57 /home/froydnj/firefox/libxul.so froydnj@hawkeye:~/src/gecko-dev.git$ size !!$ size ~/firefox/libxul.so text data bss dec hex filename 67543261 5361648 421424 73326333 45edefd /home/froydnj/firefox/libxul.so The size of the text section is what we're interested in, since that's the best representation we have of how much code size increased. (If you're so inclined, you could do `readelf -SW libxul.so |grep text` for an actual measurement of just the code.) `size` might not do the right thing on Android, because we use special compression techniques there, but it is at least worth examining. `dumpbin` is probably what you want on Windows for measuring xul.dll.
Updated•7 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 3•11 months ago
|
||
Assignee | ||
Comment 4•11 months ago
|
||
Assignee | ||
Comment 5•11 months ago
|
||
This is a relatively small code size regression (130k on windows and
macOS, 180k on Linux), for a few high confidence improvements in
speedometer 3. See link in the bug.
If this sticks, we can actually clean up a bunch of code, and eventually
unify RefPtr and nsCOMPtr. But I want this to be on the tree for a while
before doing more aggressive refactorings / actually removing the code.
Updated•11 months ago
|
Comment 6•11 months ago
•
|
||
I think we need to check apk size here too. This adds quite a bit binary overhead and we don't have too much left on apk.
Comment 7•11 months ago
|
||
For posterity, could you please write out what the important changes you see in Speedometer 3 in a comment? Presumably the PerfHerder links won't last forever. Thanks.
Comment 8•11 months ago
|
||
(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #6)
I think we need to check apk size here too. This adds quite a bit binary overhead and we don't have too much left on apk.
If this is the only issue, maybe it could be changed on a per-platform basis? eg Android shouldn't need to take a size regression for a Windows speed improvement. Having platforms diverge could cause some headaches, but on the other hand it looks like debug and opt builds are already different and I can't recall it causing huge issues. I guess the major issue would be if anybody is changing the actual nsCOMPtr class that uses it.
Comment 9•11 months ago
|
||
FWIW, the main improvement is on jQuery, TodoMVC-jQuery/total -2.02% (Windows). That is from the comment 4.
Assignee | ||
Comment 10•11 months ago
|
||
Other results show lower confidence but also general improvements.
Assignee | ||
Comment 11•11 months ago
|
||
$ du -b geckoview_example*
69062934 geckoview_example.apk
69074260 geckoview_example-with-patch.apk
So... 11.3kb, which seems not too much?
Assignee | ||
Comment 13•11 months ago
|
||
Then I'd lean towards landing the patch as-is, for simplicity and also so that we can eventually unify nsCOMPtr and RefPtr.
Comment 14•11 months ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/98796da1f171 Disable NSCAP_FEATURE_USE_BASE. r=smaug
Comment 15•11 months ago
|
||
bugherder |
Comment 16•10 months ago
|
||
It would have be good to have gotten review from another XPCOM peer for such a major change as this.
Assignee | ||
Comment 17•10 months ago
|
||
I didn't consider it particularly major fwiw, but sure... I plan to do some followup clean-ups so if it deserves more discussion we can do that there I suppose, or we can always backout if you think it's not a good direction?
Comment 18•10 months ago
|
||
Discussing it in followups sounds good to me.
Assignee | ||
Comment 19•10 months ago
|
||
(since I forgot to comment with those above)
Description
•