Closed Bug 1363754 Opened 7 years ago Closed 11 months ago

Consider removing NSCAP_FEATURE_USE_BASE

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
115 Branch
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
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?
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
Priority: -- → P3
Severity: normal → S3

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.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

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.

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.

Flags: needinfo?(emilio)

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

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

Other results show lower confidence but also general improvements.

Flags: needinfo?(emilio)
$ du -b geckoview_example*
69062934        geckoview_example.apk
69074260        geckoview_example-with-patch.apk

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

Flags: needinfo?(smaug)

That is not much

Flags: needinfo?(smaug)

Then I'd lean towards landing the patch as-is, for simplicity and also so that we can eventually unify nsCOMPtr and RefPtr.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98796da1f171
Disable NSCAP_FEATURE_USE_BASE. r=smaug
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch
Regressions: 1834693

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

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?

Discussing it in followups sounds good to me.

Blocks: 1835119

(since I forgot to comment with those above)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: