429988 - Use a background thread for places work when possible

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

Here's some work from one of my school projects.  This does the work to create a background thread, set up data for it, and that's about it.

Comment 1

[Shawn Wilsher :sdwilsh]

Comment on attachment 316764 [details] [diff] [review]
v0.1

This isn't so smart.

Comment 2

[Marco Bonardo [:mak]]

fine, work that can be moved in such a thread includes:
- livemark updates
- expiration
- db corrections (we could add some job to make sure the db is sane)
- replacing lazy messages system

what are current limitations of mozStorage writing from 2 concurrent threads? Is there any?

Comment 3

[Shawn Wilsher :sdwilsh]

Right now mozIStorageConnection isn't threadsafe, but I can fix that fairly easily in bug 448476.  The biggest issue is that statement objects can only be used on the thread they are created on.

Comment 4

[Marco Bonardo [:mak]]

what's the estimated time for the threadsafe change? Should we take temporary workaround for livemarks or wait for that?

Comment 5

[Shawn Wilsher :sdwilsh]

It would take me about an hour or two to do.  I'll start working on it now.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This adds a simple class to hold and manage the background thread.  I essentially copied what was done with the storage service.  That was designed so we could use a background thread or a thread pool - whichever we ended up needing.  This uses a single thread, but we may want to change that to a thread pool later on.

This is used by #including nsPlacesBackground.h, and dispatching events to nsPlacesBackground::target().

Looking for review from Ben for the thread stuff, and dietrich for the places stuff.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Shawn, I won't be able to get to this until next week.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 331809 [details] [diff] [review]
v1.0

the places bits are missing from this patch, removing review request.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 331809 [details] [diff] [review]
v1.0

Totally the wrong patch :(

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

The real v1.0

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 332454 [details] [diff] [review]
v1.0

Nevermind.  I fail today...

Comment 12

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This one *should* be the right one...

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 332462 [details] [diff] [review]
v1.0

This looks fine. You might want to add some comments as to why this is threadsafe (called within the places mutex, only created/destroyed on the main thread) and an NS_IsMainThread assertion couldn't hurt.

Comment 14

[Shawn Wilsher :sdwilsh]

dietrich and I were talking about doing this in JS.  The problem with that is if we ever wanted to force a sync (like when we add a bookmark) we couldn't.  We could work around that by exposing something like sync_moz_places() and sync_moz_historyvisits() in nsPIPlacesDatabase, or adding a sqlite function that those methods can call which also does the synching.

Basically I need a decision from dietrich on JS or C++ for this.

Comment 15

[Dietrich Ayala (:dietrich)]

talked over IRC. non-perf-critical code like the background tasks this is intended for should be in JS where possible for stability, maintenance, debugging reasons, as well as to lower the barrier to entry for community involvement.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 332462 [details] [diff] [review]
v1.0

Let's do this in JS then.  It'll be a new component (service) that implements nsIEventTarget.

Comment 17

[Shawn Wilsher :sdwilsh]

dietrich - so I've spent all day trying to get a JS version of this working, with no luck.  I say we go with c++ for the time being, and move to JS later when some other issues get worked out.

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

Here we go - in a nice lazy js module!

This triggers an assertion at shutdown, but this shouldn't block us (hurray for assertions not being threadsafe).  I'll file a bug on that momentarily.

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 334127 [details] [diff] [review]
v2.0

this looks ok to me, however, 2 things:

1. given that there is not a single implementation of nsIEventTarget in js in the tree, please get a second review from someone who either authored thread manager code, or is an experienced consumer of it.

2. in your patch for bug 450290, you pull in PlacesBackground.jsm. have you since changed the approach to a module? cancelling review, as that's how it's looking.

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: v2.1

Updated to what I'm actually using now.

Comment 21

[Dietrich Ayala (:dietrich)]

Comment on attachment 334497 [details] [diff] [review]
v2.1

r=me, thanks!

Comment 22

[Shawn Wilsher :sdwilsh]

Note to self:  update packages-static

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 334497 [details] [diff] [review]
v2.1

Looks good, r=me.

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: v2.2

For checkin.  I also modified the two_events_same_thread test per conversations I had with bent.

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> Note to self:  update packages-static
Do not need this for a jsm file it seems.

This can land - there are no consumers yet, but it won't get ran unless someone imports it.

Comment 26

[Shawn Wilsher :sdwilsh]

No, no it can't land.  I need to get the dependent bug landed first :/

Comment 27

[Shawn Wilsher :sdwilsh]

Using a lazy getter for thread creation isn't safe, and we could end up with two threads.

So, since the only thing folks ever want to do here is to dispatch to a thread, we should just create the thread in init.  This is OK since PlacesUtils itself uses a smart getter.

Comment 28

[Shawn Wilsher :sdwilsh]

Attachment: v2.3

Addresses that last comment I just made by making the thread creating in init instead of when it's first accessed.  The code change is trivial, so I'm not going to request review.

Comment 29

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/d353f90de69a

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking verified fixed since nothing has been backed out here.

Comment 31

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv