Closed Bug 429988 Opened 12 years ago Closed 11 years ago

Use a background thread for places work when possible

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.1b1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 7 obsolete files)

Attached patch v0.1 (obsolete) — Splinter Review
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.
Blocks: 429989
Comment on attachment 316764 [details] [diff] [review]
v0.1

This isn't so smart.
Attachment #316764 - Attachment is obsolete: true
Depends on: 448476
Assignee: nobody → sdwilsh
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?
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.
what's the estimated time for the threadsafe change? Should we take temporary workaround for livemarks or wait for that?
It would take me about an hour or two to do.  I'll start working on it now.
Attached patch v1.0 (obsolete) — Splinter Review
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.
Attachment #331809 - Flags: review?(dietrich)
Attachment #331809 - Flags: review?(bent.mozilla)
Shawn, I won't be able to get to this until next week.
Blocks: 449086
Comment on attachment 331809 [details] [diff] [review]
v1.0

the places bits are missing from this patch, removing review request.
Attachment #331809 - Flags: review?(dietrich)
Comment on attachment 331809 [details] [diff] [review]
v1.0

Totally the wrong patch :(
Attachment #331809 - Attachment is obsolete: true
Attachment #331809 - Flags: review?(bent.mozilla)
Attached patch v1.0 (obsolete) — Splinter Review
The real v1.0
Attachment #332454 - Flags: review?(dietrich)
Attachment #332454 - Flags: review?(bent.mozilla)
Comment on attachment 332454 [details] [diff] [review]
v1.0

Nevermind.  I fail today...
Attachment #332454 - Flags: review?(dietrich)
Attachment #332454 - Flags: review?(bent.mozilla)
Attached patch v1.0 (obsolete) — Splinter Review
This one *should* be the right one...
Attachment #332454 - Attachment is obsolete: true
Attachment #332462 - Flags: review?(dietrich)
Attachment #332462 - Flags: review?(bent.mozilla)
Blocks: 431558
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.
Attachment #332462 - Flags: review?(bent.mozilla) → review+
Depends on: 449884
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.
Whiteboard: [has patch][has review bent][needs review 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 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.
Attachment #332462 - Attachment is obsolete: true
Attachment #332462 - Flags: review?(dietrich)
Whiteboard: [has patch][has review bent][needs review dietrich] → [needs new patch]
No longer blocks: 449086
Blocks: 450290
Depends on: 450812
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.
Attached patch v2.0 (obsolete) — Splinter Review
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.
Attachment #334127 - Flags: review?(dietrich)
Blocks: 450913
No longer depends on: 450812
Whiteboard: [needs new patch] → [has patch][needs review dietrich]
Depends on: 450914
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.
Attachment #334127 - Flags: review?(dietrich)
Attached patch v2.1 (obsolete) — Splinter Review
Updated to what I'm actually using now.
Attachment #334127 - Attachment is obsolete: true
Attachment #334497 - Flags: review?(dietrich)
Attachment #334497 - Flags: review?(bent.mozilla)
Comment on attachment 334497 [details] [diff] [review]
v2.1

r=me, thanks!
Attachment #334497 - Flags: review?(dietrich) → review+
Whiteboard: [has patch][needs review dietrich] → [has patch][needs review bent]
Note to self:  update packages-static
Comment on attachment 334497 [details] [diff] [review]
v2.1

Looks good, r=me.
Attachment #334497 - Flags: review?(bent.mozilla) → review+
Whiteboard: [has patch][needs review bent] → [has patch][has reviews][can land]
Attached patch v2.2 (obsolete) — Splinter Review
For checkin.  I also modified the two_events_same_thread test per conversations I had with bent.
Attachment #334497 - Attachment is obsolete: true
(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.
Status: NEW → ASSIGNED
Flags: in-testsuite?
Flags: in-litmus-
Keywords: checkin-needed
No, no it can't land.  I need to get the dependent bug landed first :/
Keywords: checkin-needed
Whiteboard: [has patch][has reviews][can land] → [has patch][has reviews]
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.
Whiteboard: [has patch][has reviews] → [needs new patch]
Attached patch v2.3Splinter Review
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.
Attachment #334774 - Attachment is obsolete: true
Whiteboard: [needs new patch] → [has patch][has review]
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/d353f90de69a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
Target Milestone: --- → Firefox 3.1b1
Blocks: 449124
Marking verified fixed since nothing has been backed out here.
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.