945779 - seer thread may be taking upwards of 15% CPU during pageload

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Doug Turner (:dougt)]

we need to verify, but we have reports that Seer*Event::Run is the second hotest thread during pageload.

Comment 1

[Brendan Eich [:brendan]]

Anyone profiling?

/be

Comment 2

[Dave Herman [:dherman]]

I've posted the PowerPoint overview here:

    https://docs.google.com/presentation/d/10UIfo8q5P0KI2bADtk6gVE9bNqj5u5au1QEdC_eZQt8/edit?usp=sharing

and the Excel spreadsheet with raw data here:

    https://drive.google.com/file/d/0B8mKbzaR33BnLXB1VHdGNVhpNHM/edit?usp=sharing

Dave

Comment 3

[u408661]

So, while I was never able to reproduce the 15% number on my mac, I was able to get the seer thread to use an unreasonably high 7% of CPU time on my mac, all down to a couple extra-hot SQL statements. The statements themselves aren't particularly complex, they're just hit a *lot*. The good news is, they're fairly cacheable, so I'll work up a patch to cache the statements in question as much as possible and see what it gets us.

Comment 4

[u408661]

Attachment: patch

This patch caches the results of the biggest offender, a sql statement which was run for *every* resource on a page with results that wouldn't change until the next time the page was loaded. The next time the page loads, I just throw away the cached entry to keep the db as the sole source of truth.

This patch reduces overhead on my mac from 7% to 2% (approximately a 70% reduction in overhead).

The other main offender is a little more difficult to cache, as it's an update for each subresource on a page, but (as noted above) is a smaller percentage of the overhead on the seer thread. I can try to fix that one as well, but figured it would be better to get the bigger portion of the improvement in without blocking on more complex code.

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 8342518 [details] [diff] [review]
patch

Please don't use raw PLDHash? Look at https://developer.mozilla.org/en-US/docs/XPCOM_hashtable_guide instead.

Comment 6

[u408661]

Comment on attachment 8342518 [details] [diff] [review]
patch

That's what I get for looking at old, crufty code to figure out how to use hash tables in gecko (which I've surprisingly never done before). Would that I'd known about this at the beginning, the patch would've been ready last night :)

Thanks, jdm! I'll update to use a more modern hash table.

Comment 7

[u408661]

Attachment: patch v2

Now, with less raw PLDHashTable! Performance characteristics remain the same.

Comment 8

[Honza Bambas (:mayhemer)]

Nick, I think you must indexes for all columns you use to condition your selects ;)

There are just:

CREATE INDEX subhost_hid_origin_index ON moz_subhosts (hid, origin);
CREATE INDEX subresource_pid_uri_index ON moz_subresources (pid, uri);

but no index for moz_hosts, and I think others.  Didn't I mention it during the review?

Comment 9

[u408661]

(In reply to Honza Bambas (:mayhemer) from comment #8)
> Nick, I think you must indexes for all columns you use to condition your
> selects ;)
> 
> There are just:
> 
> CREATE INDEX subhost_hid_origin_index ON moz_subhosts (hid, origin);
> CREATE INDEX subresource_pid_uri_index ON moz_subresources (pid, uri);
> 
> but no index for moz_hosts, and I think others.  Didn't I mention it during
> the review?

You mentioned indices, but only in the specific cases that already exist. Either you and I both missed these other indices, or I misinterpreted your review comments. I'll give that a try to see how performance changes, as well.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Nicholas Hurley [:hurley] from comment #9)
> You mentioned indices, but only in the specific cases that already exist.
> Either you and I both missed these other indices, or I misinterpreted your
> review comments. I'll give that a try to see how performance changes, as
> well.

Looking at the scheme, you are selecting using columns having no indexes at all.  Just open seer.sqlite directly in sqlite3 and type |.schema|.  You will see what columns actually are primary keys, and what other indexes you create.

One example (my profile):
sqlite> explain query plan SELECT id, loads, last_load FROM moz_pages WHERE uri = "foo";
0|0|0|SCAN TABLE moz_pages (~100000 rows)

Really not good...  I think that scanning the DB might on slower devices take longer then the time you would actually do any predictive action.

Comment 11

[u408661]

Attachment: alternate patch

Here's an alternate version, with similar (slightly better in my local testing, but that could just be margin of error stuff) overhead characteristics as the caching patch. This one just adds more indices to the database as Honza pointed out should have been done a while back anyway. Both are legitimate answers (which is why I haven't obsoleted the other patch), but I think this is the "more correct" one.

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8342599 [details] [diff] [review]
patch v2

via comment 11 nick suggests going with the other patch - so dropping the review flag here. Add it again if minds change.

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 8342720 [details] [diff] [review]
alternate patch

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

honza is a better sql reviewer than me - so I'll let him do the honors. obviously more indicies adds more write overhead as the trade. you're cool with that?

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8342720 [details] [diff] [review]
alternate patch

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

I think:
- you are not using uri from page_id_uri_index ON moz_pages (id, uri)
- you are not using origin from host_id_origin_index ON moz_hosts (id, origin)

I did a rough test of this patch and monitored the number of SELECTS and INSERTS/UPDATES.  I just visited 3 pages: my blog, a bug at b.m.o and the bbc homepage.  The result is: ~400 writes, ~800 reads.  It is 1200 database operations for 3 pages!  Seems like we need to think a bit here since so much operations needed to load 3 pages will more slow down then speed up when being on lo-mem/lo-io device.

Do we have any real perf measurements for this code?

Comment 15

[u408661]

remote:   https://hg.mozilla.org/mozilla-central/rev/801fdb5306a1

Comment 16

[u408661]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> Comment on attachment 8342720 [details] [diff] [review]
> alternate patch
> 
> Review of attachment 8342720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think:
> - you are not using uri from page_id_uri_index ON moz_pages (id, uri)
> - you are not using origin from host_id_origin_index ON moz_hosts (id,
> origin)
> 
> I did a rough test of this patch and monitored the number of SELECTS and
> INSERTS/UPDATES.  I just visited 3 pages: my blog, a bug at b.m.o and the
> bbc homepage.  The result is: ~400 writes, ~800 reads.  It is 1200 database
> operations for 3 pages!  Seems like we need to think a bit here since so
> much operations needed to load 3 pages will more slow down then speed up
> when being on lo-mem/lo-io device.
> 
> Do we have any real perf measurements for this code?

Thanks, Honza. I agree, those numbers are a little insane. By my count, there should only be 5 selects to predict per page loaded (2 to look for the page and origin, 1 to see if there's any redirects, and up to 2 to get resources referenced on the page). For updating the database, there should be (2 + 4n) selects + (2 + 2n) inserts/updates per page (where n is the number of resources on a page). That number may increase by a small constant if there's a redirect on the main page or you load these pages very early after the browser starts (like, within session restore). Were you counting actual statements executed, or "steps" executed (ie, calls to statement->ExecuteStep)?

I do have some perf measurements I took on my mac using XCode's profiler, which gave me the 7% down to 2% timing numbers mentioned in comment 4. According to that data, the next biggest offender is Seer::UpdateSubresource, which is responsible for most of that 2% number on my mac. I'm looking to see if there's anything I can do to make that one faster, too.

We also have the data linked in comment 2, but since those don't have call stack data (that I can see), it's not nearly as easy to get anything useful out of.

Comment 17

[u408661]

For reference, here's a green try run from the indices patch: https://tbpl.mozilla.org/?tree=Try&rev=45cf9ddcb3e1

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Nicholas Hurley [:hurley] from comment #16)
> Thanks, Honza. I agree, those numbers are a little insane. By my count,
> there should only be 5 selects to predict per page loaded (2 to look for the
> page and origin, 1 to see if there's any redirects, and up to 2 to get
> resources referenced on the page). For updating the database, there should
> be (2 + 4n) selects + (2 + 2n) inserts/updates per page (where n is the
> number of resources on a page). That number may increase by a small constant
> if there's a redirect on the main page or you load these pages very early
> after the browser starts (like, within session restore). Were you counting
> actual statements executed, or "steps" executed (ie, calls to
> statement->ExecuteStep)?

I had tracepoints on respecitve mStatements.GetCachedStatement() what mostly reflects how often a statement is used.  If there are cycles with ExecuteStep() then those were not taken into account.

> 
> I do have some perf measurements I took on my mac using XCode's profiler,
> which gave me the 7% down to 2% timing numbers mentioned in comment 4.
> According to that data, the next biggest offender is
> Seer::UpdateSubresource, which is responsible for most of that 2% number on
> my mac. I'm looking to see if there's anything I can do to make that one
> faster, too.
> 
> We also have the data linked in comment 2, but since those don't have call
> stack data (that I can see), it's not nearly as easy to get anything useful
> out of.

Hmm.. maybe try using a transaction?  Committed in regular intervals?

Comment 19

[u408661]

(In reply to Honza Bambas (:mayhemer) from comment #18)
> I had tracepoints on respecitve mStatements.GetCachedStatement() what mostly
> reflects how often a statement is used.  If there are cycles with
> ExecuteStep() then those were not taken into account.

OK, looking at httparchive, those numbers aren't too out of whack, then, given the average number of requests per page these days. Not that it's not disturbing, and we shouldn't do more to reduce that number, just that it's not unexpected.

> Hmm.. maybe try using a transaction?  Committed in regular intervals?

Have a patch for this, waiting for the build to complete to profile it. We'll see what happens.

Comment 20

[u408661]

Attachment: transaction patch

So here's another patch, this one adding transactions as Honza suggested. Right now, I'm just committing a new transaction *after* we do the work to predict for a pageload (so we can get that work fired off ASAP), not timer-based or anything.

On my mac (still fighting my windows machine to make that useful for testing), this takes the seer thread all the way down to 0.5% (from 7% originally, and 2% after the index patch) of time usage. I've kicked off a try run at https://tbpl.mozilla.org/?tree=Try&rev=208623d64868 both for sanity checking and to get some builds for testing on the original setup to validate my code there.

Comment 21

[u408661]

Comment on attachment 8343325 [details] [diff] [review]
transaction patch

And of course, this patch has caused the tests to fail on try. Let the debugging commence...

Comment 22

[u408661]

(In reply to Nicholas Hurley [:hurley] from comment #21)
> Comment on attachment 8343325 [details] [diff] [review]
> transaction patch
> 
> And of course, this patch has caused the tests to fail on try. Let the
> debugging commence...

Turns out, the problem is a problem with the test (which wasn't a problem until we added an explicit transaction), and not the actual seer code. My inclination is to just disable the one test within test_seer.js that's causing the problem (it is, IMHO, the least important of the tests in there - it mostly exists to check my math skills, and is the most likely to be broken if we change the prediction algorithm in the future). There are a couple other options that come immediately to mind, but I'm pretty negative on both (one involves exposing an API to cause main-thread I/O, and one involves exposing an API to execute arbitrary SQL on the seer database, but at least not on the main thread).

Patrick, Honza, thoughts?

Comment 23

[Honza Bambas (:mayhemer)]

I personally vote for fixing the test.  Can the API be #ifdef TESTS'ed ?  you can get inspired at [1] and [2].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/moz.build#36.  
[2] http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/DOMStorageDBThread.cpp#685

Comment 24

[u408661]

(In reply to Honza Bambas (:mayhemer) from comment #23)
> I personally vote for fixing the test.  Can the API be #ifdef TESTS'ed ? 
> you can get inspired at [1] and [2].
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/moz.build#36.  
> [2]
> http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/
> DOMStorageDBThread.cpp#685

That seems like it might be doable, but I don't fully understand the implication of TESTS being defined. Is this something that's only enabled on tinderbox builds, and not nightlies, betas, or realease builds (I don't know how the build setups differ). If that's the case, then I'll gladly fix the tests (it'll be an easy fix).

Comment 25

[Honza Bambas (:mayhemer)]

(In reply to Nicholas Hurley [:hurley] from comment #24)
> (In reply to Honza Bambas (:mayhemer) from comment #23)
> > I personally vote for fixing the test.  Can the API be #ifdef TESTS'ed ? 
> > you can get inspired at [1] and [2].
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/moz.build#36.  
> > [2]
> > http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/
> > DOMStorageDBThread.cpp#685
> 
> That seems like it might be doable, but I don't fully understand the
> implication of TESTS being defined. Is this something that's only enabled on
> tinderbox builds, and not nightlies, betas, or realease builds (I don't know
> how the build setups differ). If that's the case, then I'll gladly fix the
> tests (it'll be an easy fix).

As I understood it when I wrote that code it is enabled only when the test is built with --enable-tests.  I presume final release builds are build w/o it.  But it might happen that I was completely wrong and the test code is in production as well :)

Comment 26

[u408661]

Attachment: transaction patch v2

OK, here's one with the test fixed. The new API to fix the test doesn't cause main-thread I/O, and the "damage" it can do is rather limited (on the off chance the API ends up enabled in a real build for real people). Other than that, this is the same as the previous version of the patch. Performance remains the same.

Comment 27

[u408661]

And here's a happy try run for the patch from comment 26 https://tbpl.mozilla.org/?tree=Try&rev=a8b0d9997ab9

Comment 28

[Honza Bambas (:mayhemer)]

Fantastic!  I'll review next week.

Comment 29

[Honza Bambas (:mayhemer)]

Comment on attachment 8343965 [details] [diff] [review]
transaction patch v2

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

I think we need the timer (followup bug OK).  Also because transactions may eat some memory.  Since you commit only on another page load and then collect (a lot) of new data, it will hang in memory until shutdown/another navigation.

This is just a quick fix for the perf issue at the expense of more memory usage (not good).

::: netwerk/base/public/nsINetworkSeer.idl
@@ +122,5 @@
>     */
>    void reset();
> +
> +  /**
> +   * @deprecated THIS API IS FOR TESTING ONLY. IF YOU DON'T KNOW WHAT IT DOES, DON'T USE IT

80 lines

thinking of an nsINetworkSeerInternal ?

::: netwerk/base/src/Seer.cpp
@@ +681,5 @@
>        NS_LITERAL_CSTRING("CREATE INDEX IF NOT EXISTS redirect_id_index "
>                           "ON moz_redirects (id);"));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  BeginTransaction();

BTW, even the table creation can be done in a transaction as well (please commit at this place and begin new trans).

@@ +2223,5 @@
> +  nsRefPtr<SeerPrepareForDnsTestEvent> event =
> +    new SeerPrepareForDnsTestEvent(timestamp, uri);
> +  return mIOThread->Dispatch(event, NS_DISPATCH_NORMAL);
> +#else
> +  return NS_ERROR_NOT_AVAILABLE;

maybe NOT_IMPLEMENTED?

::: netwerk/base/src/Seer.h
@@ +216,5 @@
>  
>    nsRefPtr<SeerDNSListener> mDNSListener;
> +
> +#ifdef SEER_TESTS
> +  friend class SeerPrepareForDnsTestEvent;

Doesn't this class need to be predeclared?

Comment 30

[u408661]

(In reply to Honza Bambas (:mayhemer) from comment #29)
> Comment on attachment 8343965 [details] [diff] [review]
> transaction patch v2
> 
> Review of attachment 8343965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we need the timer (followup bug OK).  Also because transactions may
> eat some memory.  Since you commit only on another page load and then
> collect (a lot) of new data, it will hang in memory until shutdown/another
> navigation.
> 
> This is just a quick fix for the perf issue at the expense of more memory
> usage (not good).

If we think the memory usage is going to be that bad, then I'd rather do that in this patch, given that the place the perf is going to be a big issue is on mobile devices, which is where memory is already an issue. I don't want to fix one mobile issue just to cause another :) I'll make a new patch with a timer.

> ::: netwerk/base/public/nsINetworkSeer.idl
> @@ +122,5 @@
> >     */
> >    void reset();
> > +
> > +  /**
> > +   * @deprecated THIS API IS FOR TESTING ONLY. IF YOU DON'T KNOW WHAT IT DOES, DON'T USE IT
> 
> 80 lines

Done

> thinking of an nsINetworkSeerInternal ?

We could, but I don't see this really expanding much, and not worth adding a new interface. We could maybe shove this into nsINetworkSeerVerifier, though (it ruins the naming, but that's already an internal-esque, only-useful-for-tests interface).

> ::: netwerk/base/src/Seer.cpp
> @@ +681,5 @@
> >        NS_LITERAL_CSTRING("CREATE INDEX IF NOT EXISTS redirect_id_index "
> >                           "ON moz_redirects (id);"));
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +  BeginTransaction();
> 
> BTW, even the table creation can be done in a transaction as well (please
> commit at this place and begin new trans).

Done (FWIW, I knew this, but just didn't bother for some reason or another).

> @@ +2223,5 @@
> > +  nsRefPtr<SeerPrepareForDnsTestEvent> event =
> > +    new SeerPrepareForDnsTestEvent(timestamp, uri);
> > +  return mIOThread->Dispatch(event, NS_DISPATCH_NORMAL);
> > +#else
> > +  return NS_ERROR_NOT_AVAILABLE;
> 
> maybe NOT_IMPLEMENTED?

Done

> ::: netwerk/base/src/Seer.h
> @@ +216,5 @@
> >  
> >    nsRefPtr<SeerDNSListener> mDNSListener;
> > +
> > +#ifdef SEER_TESTS
> > +  friend class SeerPrepareForDnsTestEvent;
> 
> Doesn't this class need to be predeclared?

This acts as the predeclaration (given the green builds on all platforms on try with this code).

Comment 31

[u408661]

Attachment: transaction patch v3

Updated with your previous comments, and added the timer.

Comment 32

[u408661]

Attachment: transaction patch v4

The old version was using the seer in a thread-unsafe manner, this one should fix that (only change is to initialize the timer on the main thread, so when it addrefs the seer, we don't get debug assertions firing)

Comment 33

[Richard Newman [:rnewman]]

Marking as patch-blocking Bug 947745.

Setting tracking flags to match. If this is hot enough to be noticeable on desktop, it will definitely be crushing Android phones.

Nicholas: is it worth preffing off this feature (if only on non-desktop platforms) on some or all channels until you're happy with the performance characteristics?

Comment 34

[u408661]

(In reply to Richard Newman [:rnewman] from comment #33)
> Nicholas: is it worth preffing off this feature (if only on non-desktop
> platforms) on some or all channels until you're happy with the performance
> characteristics?

See bug 948569 :)

Comment 35

[u408661]

Comment on attachment 8346615 [details] [diff] [review]
transaction patch v4

*sigh* this patch is still using seer un-thread-safely. It's pretty easily fixable, new patch incoming after I have lunch.

Comment 36

[Honza Bambas (:mayhemer)]

Nick, please obsolete any patches that are no longer valid.

Comment 37

[bhavana bajaj [:bajaj]]

(In reply to Richard Newman [:rnewman] from comment #33)
> Marking as patch-blocking Bug 947745.
> 
> Setting tracking flags to match. If this is hot enough to be noticeable on
> desktop, it will definitely be crushing Android phones.
> 
> Nicholas: is it worth preffing off this feature (if only on non-desktop
> platforms) on some or all channels until you're happy with the performance
> characteristics?

(In reply to Nicholas Hurley [:hurley] (Offline until 6 Jan 2014) from comment #34)
> (In reply to Richard Newman [:rnewman] from comment #33)
> > Nicholas: is it worth preffing off this feature (if only on non-desktop
> > platforms) on some or all channels until you're happy with the performance
> > characteristics?
> 
> See bug 948569 :)

Given the feature is disabled for Fx27 and 28 per https://bugzilla.mozilla.org/show_bug.cgi?id=948569, marking status flag accordingly and tracking this for Firefox 29 instead.

Comment 38

[u408661]

Attachment: transaction patch v5

Finally (once I got try to give me results), a patch for transactions that also has the timer in it. Honza, the only difference between this and the previous version is the timer-related commit code, everything else is the same.

happy try run: https://tbpl.mozilla.org/?tree=Try&rev=ea3ee98d37d0

Comment 39

[u408661]

Attachment: transaction patch v5 (for real this time)

Argh, by accident, the last "v5" i uploaded was actually still v4. This is the real v5.

Comment 40

[Brad Lassey [:blassey] (use needinfo?)]

Hi Nick, what are the chances that we can get a patch in for this week's beta?

Comment 41

[u408661]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #40)
> Hi Nick, what are the chances that we can get a patch in for this week's
> beta?

This feature is disabled in current beta and aurora (and will stay that way - not worth backporting), all work is happening on nightly.

Comment 42

[Brad Lassey [:blassey] (use needinfo?)]

Good to know, in that case it doesn't need to track 27. What release are you targeting to have it turned on in Beta?

Comment 43

[u408661]

The current plan is 29. Depending on review cycles, etc, there's a slight chance that could slip to 30, but it wouldn't be beyond that (and I don't even expect it to slip to 30, at least I hope not!)

Comment 44

[Honza Bambas (:mayhemer)]

Comment on attachment 8360547 [details] [diff] [review]
transaction patch v5 (for real this time)

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

r- for TYPE_REPEATING_SLACK and no cancel.

::: netwerk/base/public/nsINetworkSeer.idl
@@ +125,5 @@
> +  /**
> +   * @deprecated THIS API IS FOR TESTING ONLY. IF YOU DON'T KNOW WHAT IT DOES,
> +   * DON'T USE IT
> +   */
> +  void prepareForDnsTest(in long long timestamp, in string uri);

I would not be against having "Internal" or "Test" interface for this, up to you, not a req.

::: netwerk/base/src/Seer.cpp
@@ +294,5 @@
> +class SeerNewTransactionEvent : public nsRunnable
> +{
> +  NS_IMETHODIMP Run() MOZ_OVERRIDE
> +  {
> +    gSeer->NewTransaction();

hmm.. ok, the seer is a service, so at the time an event can fire gSeer cannot be null.  anyway some safety would be good, like let the event (and any currently existing consumers of gSeer) ref the seer when it's certainly valid.

not an r+ req for this patch.

@@ +438,5 @@
> +    nsresult rv;
> +
> +    gSeer->mCommitTimer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
> +    if (NS_SUCCEEDED(rv)) {
> +      gSeer->mCommitTimer->Init(gSeer, 5 * 1000, nsITimer::TYPE_REPEATING_SLACK);

5 * 1000 as a define or const please.

TYPE_REPEATING_SLACK - mobile people will not like you, repeated fires waste battery.  This needs to be fixed.

@@ +733,5 @@
>        NS_LITERAL_CSTRING("CREATE INDEX IF NOT EXISTS redirect_id_index "
>                           "ON moz_redirects (id);"));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  NewTransaction();

Can you please rename this to CommitAndBeginTransaction?  It's misleading.  Or use the two methods instead, whatever works better for you.

@@ +1479,5 @@
>                             predictStartTime);
>    }
>  
> +  // Use this as our signal to use a new transaction (for now)
> +  NewTransaction();

hmm.. can be removed when we have the timer?

Comment 45

[u408661]

Attachment: transaction patch v6

New version with all required (and some non-required) review comments addressed. Specifically, I didn't make a separate testing interface, nor add the extra safety checks (can be a separate bug - perhaps a good first bug for my summer intern). I removed NewTransaction in favor of explicitly calling CommitTransaction and BeginTransaction.

Comment 46

[Honza Bambas (:mayhemer)]

Comment on attachment 8361375 [details] [diff] [review]
transaction patch v6

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

r=honzab

::: netwerk/base/src/Seer.cpp
@@ +302,5 @@
> +
> +    if (!gSeer->mCommitTimer) {
> +      gSeer->mCommitTimer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
> +    } else {
> +      gSeer->mCommitTimer->Cancel();

you should get here only from the timer notification, so this might not be needed.

@@ +320,5 @@
> +  {
> +    gSeer->CommitTransaction();
> +    gSeer->BeginTransaction();
> +    nsRefPtr<SeerCommitTimerInitEvent> event = new SeerCommitTimerInitEvent();
> +    NS_DispatchToMainThread(event);

I think timers are threadsafe, up to you. (please double-check that claim if you decide to rewrite once more).

Comment 47

[u408661]

(In reply to Honza Bambas (:mayhemer) from comment #46)
> I think timers are threadsafe, up to you. (please double-check that claim if
> you decide to rewrite once more).

Timers are threadsafe, yes, but the seer can't be addref'd off the main thread, and passing it as the observer to the timer results in addrefing the seer, so the timer init has to be done on the main thread :)

I'll do one more try run on this just to make sure before landing (I've lost track of all the try runs I've done on these patches).

Comment 48

[u408661]

try run: https://tbpl.mozilla.org/?tree=Try&rev=26849f449f14

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

Sherriffs - on the off chance this gets backed out, please also back out bugs 947745 and 948448.

Comment 49

[Phil Ringnalda (:philor)]

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