612993 - 30% slower than IE9 on Shakespeare word cloud test drive page

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

All I have so far are raw numbers on Windows (32ms for m-c nightly vs 24ms for IE9pp7) and some profile data using a recent-ish TM browser build (so using -m -j -p; the scores are actually fairly insensitive to those options, though; -j is a tiny bit faster than -m).

Profile says:

  10% VM faults
  14% in tjit-generated code
  20% under js::GetPropertyByName (half of this under js_AtomizeString, 
                                   6% under js_GetPropertyHelperWithShape,
                                   rest is self time)
  17% under str_substring (half self time, 4% creating new strings, 2% floor,
                           some calls to finite())
  13% under str_IndexOf (half self time, half UnrolledMatch<ManualCmp>)
  12% under js::SetPropertyByName (3% atomizing, the rest js_SetPropertyHelper)
   8% str_unescape (half self time, half malloc(!))

Other stuff is all small.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Shell test case

Almost all time is spent under loadWords. First problem here is that source.substring(..) is about 2x slower than in IE9. Will bug 609440 help here? If not, we should probably create a new bug for it..

Comment 2

[Boris Zbarsky [:bzbarsky]]

Do you see substring() using more than about the 1/4 total time I saw it (if I assign all the vm faults to it, which is probably about right)?

Comment 3

[Jan de Mooij [:jandem]]

Attachment: substring micro-benchmark

(In reply to comment #2)
> Do you see substring() using more than about the 1/4 total time I saw it (if I
> assign all the vm faults to it, which is probably about right)?

Yes, I see ~20% under str_substring and 4.6% under vm_fault...

Most of the time substring is called with the same value for start and end. For the attached micro-benchmark I get these numbers:

- IE9 PP7: 51 ms
- Opera 11: 59 ms
- Chrome 7: 91 ms
- FF4 JM: 118 ms

If I use i and i + 1, Chrome becomes much faster (about 40 ms), the other browsers become a bit slower. For i and i+10 IE9 is still very fast (about 50 ms), Opera and Chrome become slower (70 ms and 66 ms) and Firefox gets 131 ms.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Yes, I see ~20% under str_substring and 4.6% under vm_fault...

OK, so about what I saw.  Shaving off half that time will make us 12% faster; IE is 30% faster.  So we need more than that.  ;)

But yes, seems like our substring() is comparatively slow (due to our gc sucking?).

Comment 5

[Luke Wagner [:luke]]

Bug 609440 could shave off a bit, depending on whether ropes are involved.  If ropes *are* involved, however, it's bug 615280 that would make a huge difference.  Is any time being spent in JSString::flatten (since flatten is defined in jsstr.cpp with str_substring, perhaps its being inlined)?

Other than that, I see str_substring is doing two ValueToNumber -> js_DoubleToInteger conversions which is begging for a v.isInt32() fast path.

Comment 6

[Tom S. (please needinfo tschuster)]

I tested this out, replaced all double usages and made the bounds-checking more sane. I propably introduced with special doubles, need to check this.

js -m before:
142
js -m after
48

Comment 7

[Jan de Mooij [:jandem]]

If I comment out this line: addWord( source.substring( idx, ns )); 
JM is as fast as IE9 (4.4 ms). JM+TM is a bit slower (4.9 ms)

The indexOf is a bit faster in FF. Without that there is not much left, addWord may cost another ms but that's hard to measure reliably without substring.

I will profile the other part (makeCloudByOccurance) later. We're 1ms slower than IE9 (3 vs 4) so that won't help our score much.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to comment #6)
> I tested this out, replaced all double usages and made the bounds-checking more
> sane. I propably introduced with special doubles, need to check this.

Cool, how does -m (on the shell test case attached here) compare to IE9?

Comment 9

[Tom S. (please needinfo tschuster)]

Attachment: substring patch

This is not really easy to understand, especially i might have some bug in double handling, or the case end < begin.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to comment #9)
> Created attachment 494774 [details] [diff] [review]
> substring patch

Nice, you want to finish this? (maybe in a separate bug blocking this one?) str_substr has the same problem btw...

Comment 11

[Brendan Eich [:brendan]]

Tom, if you can't get to this, please let us know -- would be great to keep it moving along.

/be

Comment 12

[Tom S. (please needinfo tschuster)]

Sorry forget to link bug 616612 here.

Comment 13

[Tom S. (please needinfo tschuster)]

On this computer, we are faster for "Cymbeline", 28.6ms vs 29.5ms in IE9.

Comment 14

[Guilherme Lima]

"Cymbeline"
IE10: 24.7ms
Firefox Nightly 24: 29.5ms
Google Chrome 27: 31.4ms

Comment 15

[Guilherme Lima]

"Cymbeline"
Chrome 59: 16.6ms
IE11: 22.2ms
Nightly 56: 25.0ms
Firefox 54: 40.3ms

Comment 16

[Gregory Pappas [:gregp]]

Firefox and Chromium seem to perform identically in this case now.

Do you think this bug is safe to close?

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Gregory Pappas [:gregp] from comment #16)

Firefox and Chromium seem to perform identically in this case now.

Do you think this bug is safe to close?

Yes that makes sense. We've come a long way :)