1297486 - don't MOZ_ALWAYS_INLINE xpc::NonVoidStringToJsval

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: 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?

Comment 1

[Boris Zbarsky [: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.

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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>

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

I meant:

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

of course.  ;)

Comment 6

[Nathan Froyd [:froydnj]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

> It looks like the MOZ_ALWAYS_INLINE version is ~2-3% faster on that microbenchmark

What were the actual numbers you were seeing?

Comment 8

[Nathan Froyd [:froydnj]]

(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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: mark xpc::NonVoidStringToJsval as an ordinary inline function instead of MOZ_ALWAYS_INLINE

The obvious patch, reviewed by bz already.

Comment 11

[Nathan Froyd [:froydnj]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d1bd8955e9e