1343172 - [e10s] Performance regression, Computing hash on virustotal is slow. Nightly54.0a1 is 5-10 times slower than non e10s

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]:

Steps to Reproduce:
1. Open https://www.virustotal.com/en/
2. Chose one of exe file from local filt system (e.g. full installer of Firefox)
3. Clock on [Scan it!], (no needs actual upload)
   --- observe, Computing hash...

Actual Results:
Computing hash is slow on Nightly54.0a1
Nightly54.0a1 is 5-10 times slower than Aurora53.0a2.

Expected Results:
It should be same as Aurora53.0a2

Comment 1

[Jan de Mooij [:jandem]]

Do you have a regression range?

Comment 2

[Alice0775 White]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Do you have a regression range?

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d74a9133f9a5c0a7374bef2a0be15e3ed7b37113&tochange=41d7a4864f3325cd9abcc5c5a17788334c8ca863

a11y is activated on my PC due to ATOK IME.
e10s off + a11y on(due to ATOK)  : fix, it is fast.
e10s on  + a11y off(Disable ATOK) : not fix.
e10s on  + a11y force off : not fix.


(It may not be related to JS. However, it is strange. Why does e10s affect to computing hash?).


Gecko profiler:
https://perfht.ml/2moFKf6

Comment 3

[Jan de Mooij [:jandem]]

Hm, it's probably unrelated to Aaron's patches but more of an e10s issue.

Milan, the profile (comment 2) shows a lot of time under PCompositorBridgeChild::OnMessageReceived. Any idea what's going on there?

Comment 4

[Alice0775 White]

BTW, It is affected to 50+, if there is no add-on installed and no a11y application. Because, e10s is enabled by default.

Comment 5

[Alice0775 White]

Attachment: https://www.virustotal.com/static/js/sha256.js

Comment 6

[Alice0775 White]

Attachment: reduced.html

Comment 7

[Alice0775 White]

FYI, Bug 1342769 does not fix this issue.

Comment 8

[Alice0775 White]

Okay, FileReaderSync.readAsBinaryString is 50 times slower in e10s.

Comment 9

[Gerry Chang [:gchang]]

Track 54+ as performance regression.

Comment 10

[Jan de Mooij [:jandem]]

baku, needinfo ping?

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrew, Nathan, this sounds potentially bad for 54. Should this be more of a priority to investigate? 
Looks like we have more detailed info in comment 8.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: reader.patch

Here we are doing something odd. We are asking for a stream also when we have it, and then we ignore the new stream in SetStream.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> Andrew, Nathan, this sounds potentially bad for 54. Should this be more of a
> priority to investigate? 
> Looks like we have more detailed info in comment 8.

Andrea has a patch--thank you Andrea!  We should definitely try to get this uplifted sooner rather than later.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7397d7229b32
PBlob should not create a remoteInputStream if that already exists, r=smaug

Comment 15

[Andrew Overholt [:overholt]]

baku, please be sure to ask for uplift here.

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8858552 [details] [diff] [review]
reader.patch

Approval Request Comment
[Feature/Bug causing the regression]: PBlob
[User impact if declined]: FileReader, when used in workers can be extremely slow.
[Is this code covered by automated tests?]: none
[Has the fix been verified in Nightly?]: manually, yes
[Needs manual test from QE? If yes, steps to reproduce]: yes, would be nice to have it tested.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: none
[Why is the change risky/not risky?]: instead recreating the stream each time we do it only onces. 
[String changes made/needed]: none

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7397d7229b32

Comment 18

[Gerry Chang [:gchang]]

Hi Alice, 
can you help verify if this issue is fixed as expected?

Comment 19

[Alice0775 White]

I can reproduced the problem on Nightly55.0a1(2017-04-18)[1].
[1]https://hg.mozilla.org/mozilla-central/rev/bb38d935d699e0529f9e0bb35578d381026415c4
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 ID:20170418030220

And I verified as fixed on Nightly55.0a1(2017-04-19)[2].
[2]https://hg.mozilla.org/mozilla-central/rev/c0ea5ed7f91a6be996a4a3c5ab25e2cdf6b4377e
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 ID:20170419030223

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8858552 [details] [diff] [review]
reader.patch

Fix a perf regression and was verified. Beta54+. Should be in 54 beta 1.

Comment 21

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cec324879dca - Andrea Marchesini - Bug 1343172 - PBlob should not create a remoteInputStream if that already exists, r=smaug. a=gchang

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8858552 [details] [diff] [review]
reader.patch

See comment 16.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8858552 [details] [diff] [review]
reader.patch

User impact from comment 16 sounds severe, SWs, e10s are both new in ESR52, A+

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c7b064f6b93a

Comment 25

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using old Nightly from 2017-02-28 on Windows 10, verified that the issue does not reproduce anymore using latest Firefox 54 beta 9 across platforms (Windows 10, macOS 10.12.4 and Ubuntu 16.04 32bit).