don't MOZ_ALWAYS_INLINE xpc::NonVoidStringToJsval

RESOLVED FIXED in Firefox 51

Status

()

Core
XPConnect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
Created attachment 8784088 [details]
disassembly of NonVoidStringToJsval caller with and without MOZ_ALWAYS_INLINE

We mark xpc::NonVoidStringToJsval as MOZ_ALWAYS_INLINE.  This function generates a lot of code; I've attached a disassembly of a binding attribute getter with NonVoidStringToJsval inlined and without it inlined.  The size difference is pretty large, and it's not obvious to me that we benefit from having all that code inline at every single callsite.  NonVoidStringToJsval has a bunch of conditionals, and requires several calls into the JS engine (particularly the GC) to do its thing.  It might have made sense once upon a time, but I think it's time to revisit that decision.

Marking it as merely |inline| and letting the compiler make some kind of call about whether to inline cuts libxul.so size on x86-64 Linux by over 550K, which is...hefty.  (libxul.so is ~70MB or so.)

bz, WDYT?
Flags: needinfo?(bzbarsky)
How is the performance of getAttribute and the id getter affected?  Please make sure to comment out the [Pure] annotations on those when testing....

Note that the fast path through NonVoidStringToJsval is that HasStringBuffer() tests true, length != 0, and we do StringBufferToJSVal (the always inlined version), which in the common case does GetContextZone() (inlined) and then JS_GetZoneUserData (ool) and then some other bit twiddling, but the only OOL bit is JS_GetZoneUserData. The GC bits are also always-inline.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 2

a year ago
Bug 834877 used dromaeo for performance numbers, so I'm going to do the same here (Linux x86-64):

http://dromaeo.com/?id=253289,253288

The first column is the current code (MOZ_ALWAYS_INLINE), the second column is with the proposed change.  These tests were also run with the [Pure] annotation in Element.webidl on getAttribute and id commented out.

DOM attributes overall gets 2% slower with these changes, though getAttribute and element.property are slightly faster.  DOM tests overall are 10% faster, though that's entirely due to the DOM Query numbers, which I'm not entirely sure I believe.  I tried running these on a pretty quiet system, but it's possible that something in the browser itself (deferred tasks after startup, ...) might have botched the numbers a bit.

Assuming those stay stable in repeated testing, does that seem reasonable?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 3

a year ago
Different set of tests with a "quiescient" browser that was started up and allowed to sit for a while:

http://dromaeo.com/?id=253293,253295

The first column is the current code (MOZ_ALWAYS_INLINE), the second column is with the proposed change.  Again, no [Pure] annotation.

Overall scores are about the same, and so are most of the tests.
OK.  I'd love to see numbers for this specific microbenchmark too, if you have both builds lying around:

  <div foo="bar"></div>
  <script>
    var div = document.querySelector("div");
    var count = 1000000;
    var start = performance.now();
    for (var i = 0; i < count; ++count) {
      div.getAttribute("foo");
    }
    var stop = performance.now();
    document.writeln((stop - start)/count*1e6);
  </script>
Flags: needinfo?(bzbarsky)
>    for (var i = 0; i < count; ++count) {

I meant:

    for (var i = 0; i < count; ++i) {

of course.  ;)
(Assignee)

Comment 6

a year ago
It looks like the MOZ_ALWAYS_INLINE version is ~2-3% faster on that microbenchmark, which is at least consistent with the Dromaeo numbers.
> It looks like the MOZ_ALWAYS_INLINE version is ~2-3% faster on that microbenchmark

What were the actual numbers you were seeing?
(Assignee)

Comment 8

a year ago
(In reply to Boris Zbarsky [:bz] from comment #7)
> What were the actual numbers you were seeing?

34.50 (averaged over several runs) for MOZ_ALWAYS_INLINE
35.47 for plain inline
r=me to nix the MOZ_ALWAYS_INLINE.

I'd be curious to see what times look like for just making it out-of-line too.
(Assignee)

Comment 10

a year ago
Created attachment 8784525 [details] [diff] [review]
mark xpc::NonVoidStringToJsval as an ordinary inline function instead of MOZ_ALWAYS_INLINE

The obvious patch, reviewed by bz already.
Attachment #8784525 - Flags: review+
Assignee: nobody → nfroyd
(Assignee)

Comment 11

a year ago
(In reply to Boris Zbarsky [:bz] from comment #9)
> r=me to nix the MOZ_ALWAYS_INLINE.
> 
> I'd be curious to see what times look like for just making it out-of-line
> too.

AFAICT--grepping through libxul disassembly for instruction patterns unique to NonVoidStringToJsval, like calling JS_SetZoneUserData--declaring it |inline| here is basically equivalent to declaring it out-of-line...at least on GCC.

Comment 12

a year ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1bd8955e9e
mark xpc::NonVoidStringToJsval as an ordinary inline function instead of MOZ_ALWAYS_INLINE; r=bz

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d1bd8955e9e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.