1363754 - Consider removing NSCAP_FEATURE_USE_BASE

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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

[Nathan Froyd [:froydnj]]

(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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=828a9fee611e9c22347dbb8e55ea1d1e9ccbddc9&newProject=try&newRevision=2ef88dea932af3da09cbff7432a0ff09446875d8&page=1

Comment 4

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=136889a7ff3312510119369c4ce47c129520fce5&newProject=try&newRevision=5410be7b4489871f1959e73836ae76489e5d30a2

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1363754 - Disable NSCAP_FEATURE_USE_BASE. r=smaug,#xpcom-reviewers

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

[Andrew McCreight [:mccr8]]

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

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug][[e-mail]] 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

[Olli Pettay [:smaug][[e-mail]]]

FWIW, the main improvement is on jQuery, TodoMVC-jQuery/total -2.02% (Windows). That is from the comment 4.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: non-low-confidence windows results.

Other results show lower confidence but also general improvements.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

$ du -b geckoview_example*
69062934        geckoview_example.apk
69074260        geckoview_example-with-patch.apk

So... 11.3kb, which seems not too much?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

That is not much

Comment 13

[Emilio Cobos Álvarez (:emilio)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98796da1f171
Disable NSCAP_FEATURE_USE_BASE. r=smaug

Comment 15

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/98796da1f171

Comment 16

[Andrew McCreight [:mccr8]]

It would have be good to have gotten review from another XPCOM peer for such a major change as this.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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

[Andrew McCreight [:mccr8]]

Discussing it in followups sounds good to me.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: XUL section size differences for posterity.

(since I forgot to comment with those above)