Last Comment Bug 778855 - Talos Ts regressions from bug 769960 (SafeBrowsing.jsm refactor)
: Talos Ts regressions from bug 769960 (SafeBrowsing.jsm refactor)
Status: RESOLVED FIXED
[ts][leave open]
: perf, regression
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
Depends on:
Blocks: 769960
  Show dependency treegraph
 
Reported: 2012-07-30 12:33 PDT by Matt Brubeck (:mbrubeck)
Modified: 2014-05-27 12:25 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch v.1 (1.20 KB, patch)
2012-07-30 17:47 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Review

Description Matt Brubeck (:mbrubeck) 2012-07-30 12:33:14 PDT
It looks like bug 769960 regressed Ts Paint and Ts MAX/MED Dirty Profile benchmarks by 3% to 6% on all desktop platforms, and also made the results noisier, e.g.:
http://graphs.mozilla.org/graph.html#tests=[[83,63,21]]&sel=1343376589000,1343549389000&displayrange=7&datatype=running

Did we accidentally move some code to run before startup that used to run after startup?
Comment 1 Justin Dolske [:Dolske] 2012-07-30 15:44:31 PDT
Hmm, yeah, this did move some code to run earlier than it did before.

The pre-refactoring code (take a deep breath) used a <script> in browser.xul, which added a |load| event listener, which in turn added a 2 second setTimeout, which in turn finally did the actual init. See (now deleted) sb-loader.js.

The refactored code simply init()s in gBrowserInit._delayedStartup(). Which instead of 2 seconds after |onload|, is 1 event-loop spin. Some of this regression / noise may be due to us writing to a DB to add the Mozilla test entries:

http://mxr.mozilla.org/mozilla-central/source/browser/components/safebrowsing/SafeBrowsing.jsm#180

Lots of (existing) clownshoes in this code. Sigh.

Let me look at removing addMozEntries(). Failing that, we can go back to cheating by initializing after another 2 seconds.
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-30 15:54:42 PDT
Assuming that dolske meant to take this!
Comment 3 Justin Dolske [:Dolske] 2012-07-30 17:38:44 PDT
Filed bug 779008 for the DB clownshoes. It's existing, needs a bit of thought, and it just one of many undesirable things with Safe Browsing.

So I'm just going to fix this regression by bumping the init() to occur later, as the original code did.
Comment 4 Justin Dolske [:Dolske] 2012-07-30 17:47:00 PDT
Created attachment 647375 [details] [diff] [review]
Patch v.1

This restores init timing to what it was pre-refactor.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-30 17:53:12 PDT
Comment on attachment 647375 [details] [diff] [review]
Patch v.1

gross
Comment 6 Justin Dolske [:Dolske] 2012-07-30 22:58:39 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/19c4aff72747

We'll probably want to watch numbers for a bit after landing to verify that it's indeed fixed.
Comment 7 Ed Morley [:emorley] 2012-07-31 06:08:35 PDT
https://hg.mozilla.org/mozilla-central/rev/19c4aff72747
Comment 8 Justin Dolske [:Dolske] 2012-07-31 13:15:29 PDT
http://graphs.mozilla.org/graph.html#tests=[[83,63,21]]&sel=1343354155024.9382,1343765668427&displayrange=7&datatype=running

Looks like this fixed it. Noise is gone, and Ts is actually a little lower than it was before!

Note You need to log in before you can comment on or make changes to this bug.