Closed
Bug 429988
Opened 16 years ago
Closed 16 years ago
Use a background thread for places work when possible
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3.1b1
People
(Reporter: sdwilsh, Assigned: sdwilsh)
References
Details
Attachments
(1 file, 7 obsolete files)
8.86 KB,
patch
|
Details | Diff | 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.
Assignee | ||
Comment 1•16 years ago
|
||
Comment on attachment 316764 [details] [diff] [review] v0.1 This isn't so smart.
Attachment #316764 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → sdwilsh
Comment 2•16 years ago
|
||
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?
Assignee | ||
Comment 3•16 years ago
|
||
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•16 years ago
|
||
what's the estimated time for the threadsafe change? Should we take temporary workaround for livemarks or wait for that?
Assignee | ||
Comment 5•16 years ago
|
||
It would take me about an hour or two to do. I'll start working on it now.
Assignee | ||
Comment 6•16 years ago
|
||
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.
Comment 8•16 years ago
|
||
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)
Assignee | ||
Comment 9•16 years ago
|
||
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)
Assignee | ||
Comment 10•16 years ago
|
||
The real v1.0
Attachment #332454 -
Flags: review?(dietrich)
Attachment #332454 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 11•16 years ago
|
||
Comment on attachment 332454 [details] [diff] [review] v1.0 Nevermind. I fail today...
Attachment #332454 -
Flags: review?(dietrich)
Attachment #332454 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 12•16 years ago
|
||
This one *should* be the right one...
Attachment #332454 -
Attachment is obsolete: true
Attachment #332462 -
Flags: review?(dietrich)
Attachment #332462 -
Flags: review?(bent.mozilla)
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+
Assignee | ||
Comment 14•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch][has review bent][needs review dietrich]
Comment 15•16 years ago
|
||
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.
Assignee | ||
Comment 16•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch][has review bent][needs review dietrich] → [needs new patch]
Assignee | ||
Comment 17•16 years ago
|
||
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.
Assignee | ||
Comment 18•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs new patch] → [has patch][needs review dietrich]
Comment 19•16 years ago
|
||
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)
Assignee | ||
Comment 20•16 years ago
|
||
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 21•16 years ago
|
||
Comment on attachment 334497 [details] [diff] [review] v2.1 r=me, thanks!
Attachment #334497 -
Flags: review?(dietrich) → review+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch][needs review dietrich] → [has patch][needs review bent]
Assignee | ||
Comment 22•16 years ago
|
||
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+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch][needs review bent] → [has patch][has reviews][can land]
Assignee | ||
Comment 24•16 years ago
|
||
For checkin. I also modified the two_events_same_thread test per conversations I had with bent.
Attachment #334497 -
Attachment is obsolete: true
Assignee | ||
Comment 25•16 years ago
|
||
(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.
Assignee | ||
Comment 26•16 years ago
|
||
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]
Assignee | ||
Comment 27•16 years ago
|
||
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]
Assignee | ||
Comment 28•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs new patch] → [has patch][has review]
Assignee | ||
Comment 29•16 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/d353f90de69a
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
Target Milestone: --- → Firefox 3.1b1
Comment 30•15 years ago
|
||
Marking verified fixed since nothing has been backed out here.
Status: RESOLVED → VERIFIED
Comment 31•15 years ago
|
||
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.
Description
•