890524 - Big browser-only Kraken crypto-aes regression since Firefox 21

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Bug 833898 caused a big browser-only Kraken crypto-aes regression (> 2x slower). I also see the regression if I run just this test in the shell, but unfortunately when running all Kraken tests it doesn't show up so we didn't notice it.. I have no idea why, it may be GC related somehow. It's really unfortunate we didn't see this earlier - we should compare browser vs shell numbers more often.

The problem is that if we have a SETELEM with AmbiguousDoubleConversion, some objects require double conversion but for other objects it's invalid to convert to double. So we use an IC instead of inlining the store directly (and this IC does not attach a stub). This patch adds a new instruction that checks if the array's elements header has the CONVERT_DOUBLE_ELEMENTS flag set and only in that case it converts the value to double.

For now this handles only integer values - I think that's the most common case and making it work for all values will require factoring out LValueToDouble. I hope we can uplift this to aurora so I want to keep the patch as simple as possible.

I did a Try push and this patch fixes the problem:

    crypto-aes:              2.09x as fast      207.9ms +/- 2.1%     99.6ms +/-

Hannes, if you're not comfortable reviewing this, feel free to forward to Brian.

Comment 1

[Guilherme Lima]

This fixes the crypto-aes regression reported in https://bugzilla.mozilla.org/show_bug.cgi?id=849325

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 771650 [details] [diff] [review]
Patch

Review of attachment 771650 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch! Seems pretty straightforward to what it does.

The only thing that bothers me is the difference between "MaybeConvertToDoubles" and "AmbiguousDoubleConversion".
Currently we convert "MaybeConvertToDoubles" to "AlwaysConvert" and for "AmbiguousDoubleConversion" we now use "MMaybeConvertToDoubles".
This is confusing... Should we find another name for "types::StackTypeSet::MaybeConvertToDoubles" ??

Comment 3

[Jan de Mooij [:jandem]]

Renamed MaybeConvertToDoubleElement to MaybeToDoubleElement as discussed on IRC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf84b4a4e663

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf84b4a4e663

Comment 5

[Florian Bender]

Should/can this be uplifted? It's still early in the cycle, and 24 will be ESR so there may be enough reasons to warrant an uplift.