Closed Bug 429988 Opened 12 years ago Closed 11 years ago
Use a background thread for places work when possible
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 on attachment 316764 [details] [diff] [review] v0.1 This isn't so smart.
Attachment #316764 - Attachment is obsolete: true
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.
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.
Shawn, I won't be able to get to this until next week.
Comment on attachment 331809 [details] [diff] [review] v1.0 the places bits are missing from this patch, removing review request.
Comment on attachment 331809 [details] [diff] [review] v1.0 Totally the wrong patch :(
The real v1.0
Comment on attachment 332454 [details] [diff] [review] v1.0 Nevermind. I fail today...
This one *should* be the right one...
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+
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.
Whiteboard: [has patch][has review bent][needs review dietrich] → [needs new patch]
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.
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.
Whiteboard: [needs new patch] → [has patch][needs review 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.
Updated to what I'm actually using now.
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]
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
No, no it can't land. I need to get the dependent bug landed first :/
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]
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
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.