1223153 - 21% MacOS sessionrestore on Mozilla-Inbound (v.45) on November 07, 2015 from push c99a26fcff8f7ecb57b379e2bbe909c3844c2b69

Status: RESOLVED FIXED

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

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Talos has detected a Firefox performance regression from your commit c99a26fcff8f7ecb57b379e2bbe909c3844c2b69 in bug 1003439.  We need you to address this regression.

This is a list of all known regressions and improvements related to your bug:
http://alertmanager.allizom.org:8080/alerts.html?rev=c99a26fcff8f7ecb57b379e2bbe909c3844c2b69&showAll=1

On the page above you can see Talos alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test, please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#sessionrestore.2Fsessionrestore_no_auto_restore

Reproducing and debugging the regression:
If you would like to re-run this Talos test on a potential fix, use try with the following syntax:
try: -b o -p macosx64 -u none -t other[10.10]  # add "mozharness: --spsProfile" to generate profile data

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

Then run the following command from the directory where you set up Talos:
talos --develop -e <path>/firefox -a sessionrestore_no_auto_restore

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Thursday, or the offending patch will be backed out! ***

Our wiki page outlines the common responses and expectations:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

sessionrestore* tests in e10s/non-e10s are regressed on osx 10.10:
https://treeherder.mozilla.org/perf.html#/graphs?series=[mozilla-inbound,a9e66ff65e0a8438260f480c2cb2e3814bcb59c2,1]&series=[mozilla-inbound,31bd26f64638a428fd20cf5c6e4e41891e9f7847,1]&series=[mozilla-inbound,b2e46651909db5bbec67bf987a6d2b09daad10cb,1]&series=[mozilla-inbound,d83aa21cf592c17bba37edab93a678b4b29acd53,1]&highlightedRevisions=bf3b50448aa1&highlightedRevisions=c99a26fcff8f

In addition ts_paint (non-e10s) shows a regression:
https://treeherder.mozilla.org/perf.html#/graphs?series=[mozilla-inbound,5cef704e0841c8a7b338d0377091681f1150fd16,1]&highlightedRevisions=bf3b50448aa1&highlightedRevisions=c99a26fcff8f

:eeejay, can you take a look at this and comment on why we are seeing regressions on osx 10.10 only and appears to be on startup tests specifically?  If this is fixable we should fix it!

Comment 2

[Eitan Isaacson [:eeejay]]

Mokoto,

Could you look at the mac speech service you wrote and see why startup time is hindered?

Comment 3

[Makoto Kato [:m_kato]]

Humm, Mac has a lot of voice items and API call will be slow.  I should enumerate it on non Gecko main thread.

Comment 4

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=634625f7e7dc

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Create worker thread to enumerate voice items

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 8686064 [details] [diff] [review]
Create worker thread to enumerate voice items

When startup, OSX Speech Synthesizer service enumerates all voice items.  But this call is expensive (Objective-C API call and item is 64).

So I should create worker thread to enumerate items, then register it on gecko main thread

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 8686064 [details] [diff] [review]
Create worker thread to enumerate voice items

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

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm
@@ +290,4 @@
>    }
>  
> +  RefPtr<RegisterVoicesRunnable> runnable = new RegisterVoicesRunnable(mSpeechService, list);
> +  NS_DispatchToMainThread(runnable, NS_DISPATCH_SYNC);

I bet you could use NS_NewRunnableMethodWithArgs if you didn't want to manually create another runnable class, but this works too.

@@ +326,5 @@
>      return false;
>    }
>  
> +  nsCOMPtr<nsIThread> thread;
> +  if (NS_FAILED(NS_NewNamedThread("SpeechWokrer", getter_AddRefs(thread)))) {

typo, SpeechWorker

Comment 8

[Pulsebot]

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

Comment 9

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

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

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

tit is hard to tell if we are 100% fixed, but I will say that it looks much better.  Thanks for fixing this!