When I tap on the bookmark button, it stays pushed in with nothing else happening for a a second or two. Eventually, the star turns yellow, the button pops back out, and the "Page Bookmarked" popup comes up, but there's definitely a delay that there didn't used to be.

I can't seem to reproduce this, and Madahava can't either. I _can_ reproduce an exception being thrown on the device in certain cases: Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getFolderIdForItem]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: file:///home/user/fennec/xulrunner/modules/utils.js :: PU_getMostRecentBookmarkForURI :: line 1014" data: no]

I guess that's probably related to bug 515757.

Are you guys using the same device, I saw this when bookmarking about:buildconfig and planet.mozilla.org on build: Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091008 Fennec/1.0b4 I'm seeing a 1-2 second delay, where the button is shown in a pressed state, but the pop-up edit/remove dialog is not shown and the star is still a hollow-white.

(In reply to comment #2) > I guess that's probably related to bug 515757. Yeah. There is no reason for this code to failed on device excepted places performances. I mean the getFolderIdForItem failed, but if I'm doing in a stuff like that it works: while(failed) { try { getFolderIdForItem(item); failed = false; } catch(e) { } } I'm not really sure of what we can do here except prevent click on the button.

(In reply to comment #3) > Are you guys using the same device, I saw this when bookmarking > about:buildconfig and planet.mozilla.org on build: Can't reproduce on my n810. The times to bookmark these pages are from 50ms to 200ms in the worst case.

Confirmed this is still happening on builds: Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b4pre) Gecko/20091123 Firefox/3.6b4pre Fennec/1.0b6pre and Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091123 Firefox/3.7a1pre Fennec/1.0b5

I can reproduce this on an n900. It happens for me on wikipedia.org, for one. The page is unbookmarked, I hit the bookmark button and it stays depressed for more than a second. Actually, it happens reliably for every site I test. I've turned off all of my add-ons and the problem persists.

(In reply to comment #7) > Actually, it happens reliably for every site I test. I've turned off all of my > add-ons and the problem persists. I know you have weave installed, and have a lot of bookmarks. I wonder if this is a function of the number of bookmarks in a system. Can you try this with a new, empty profile?

Ah, good question. Let me try. Nope -- same problem with a fresh profile.

This is entirely because of insertBookmark and is all I/O bound because of SQL calls. There aren't really any quick fixes for this in 1.9.2. We could try playing with PRAGMA synchronous on just maemo (http://www.sqlite.org/pragma.html), but Shawn says this caused a lot of corruption issues on desktop. I suggest we take this off the blocking list.

This is still happening to me...re-nominating

IMO there's no way this will be fixed for 1.1. The async places work is still ongoing.

Resetting assignee, I don't work on this bug.

Marco believes that DBFlush may be killing performance of insertBookmark, which will go away when we kill temp tables. Adding as a dependency; we should revisit this bug once it has landed. If this doesn't help enough, maybe we could add a completely separate call for inserting bookmarks asynchronously?

Adding myself to CC to track the progress.

(In reply to comment #15) > If this doesn't help enough, maybe we could add a completely separate call for > inserting bookmarks asynchronously? It's very likely not going to help enough and what's killing you is the main thread disk I/O. We shouldn't wait and see if bug 552023 is going to help and just make an async API for adding a bookmark which will absolutely fix this.

Per comment 17, morphing. This could use an owner (read: not places team) if you want it done anytime soon.

Lack of this means that Sync causes main thread I/O, which is sadfaces.

What needs to be done here is this: We need a new method on nsINavBookarksService (or something new) called insertBookamrkAsync. IDL would look just like nsINavHistory::insertBookmark expect we would not return the bookmark id (so it would be a void method). If consumers need to know the id, they can use a bookmark observer. However, this may not work for Sync since onItemAdded doesn't also pass the URI. This may make it difficult to associate the id, if needed, to the URI. Input from the sync team is needed (mak should chime in here too). All this work should be done in one shot using an nsIRunnable, like we are making history visits do in bug 599969. All this work should happen on the places branch, not mozilla-central, as there have been recent changes there that are incompatible with mozilla-central.

And if we want to return the id asynchronously, we can add one more optional argument at the end of the method that will be a callback implementing something like this interface: nsIPlacesIdCallback : public nsISupports { /** * Called when an item is added to history or bookmarked. * * @param aId * The id of the history visit or bookmark that was added. * @param aURI * The URI that was added. */ void onItemAdded(in long long aId, in nsIURI aURI); }

We should go with the optional callback because we'll also want to use it for history visits, so it makes sense to go ahead and make them both the same (bug 606966 comment 5)

hm so if something implements both nsIBookmarksObserver and nsIPlacesIdCallback it will get double onItemAdded notifications? What's bad with adding the uri to the existing onItemAdded?

Comment on attachment 485822 [details] [diff] [review] interface changes >+[scriptable, uuid(4ae42b90-2b84-4d56-833f-53fa22d43767)] >+interface nsIPlacesIdCallback : nsISupports >+{ >+ /** >+ * Called when an item is added to history or bookmarked. >+ * >+ * @param aId >+ * The id of the history visit or bookmark that was added. >+ * @param aURI >+ * The URI that was added. >+ */ >+ void onItemAdded(in long long aId, in nsIURI aURI); >+}; We'd want this in its own file though. This is what I'm thinking of though, yeah.

(In reply to comment #24) > hm so if something implements both nsIBookmarksObserver and nsIPlacesIdCallback > it will get double onItemAdded notifications? I think this is a lighter weight callback and you may not need both in most situations, but we could certainly call it something else. > What's bad with adding the uri to the existing onItemAdded? Mostly us changing our API this late (I think it'd have to happen for beta 7, which seems like a lot of work to rush).

I suggest to not reuse same method names as nsIBookmarksObserver. if possible the callback should also be marked as [function], but that can only happen if the callback object has a single function in it, if we could make a single onItemUpdated(aActionPerformed, aItemId, aType, aParentId, aIndex, aURI) this would coalesce practically all items of nsINavBookmarkObserver methods but batches, onItemVisited and OnItemChanged. We should know all these infos in most/all cases. Btw, talking on IRC looks like Sync could not need to get back a callback at all since they use GUIDs

I am not going to implement callbacks. we can do that as a followup.

This is also going to need changes to mobile-browser, right?

and to services/sync. Lets do that as follow-ups and/or separate patches.

(In reply to comment #33) > and to services/sync. Lets do that as follow-ups and/or separate patches. We have bug 606353 for the Sync part.

Comment on attachment 485906 [details] [diff] [review] core changes Most of the APIs that are used by nsNavBookmarks::InsertBookmark are not safe to use off of the main thread. You should be asserting like mad in a debug build here. We actually have to re-implement nsNavBookmarks::InsertBookmark in a threadsafe way and cannot just call the old code.

I didn't see any asserts. But I dont recall if it was a debug build or not. What parts are not threadsafe. Where are the critical sections?

(In reply to comment #36) > What parts are not threadsafe. Where are the critical sections? Well, all the statements cannot be used on different threads. The observers cannot be notified on a background thread. Those were the first two things I noticed that weren't right and didn't look much further.

well, that is a pretty huge fan out of calls. :/ has any work been done to make any of this threadsafe (like in pending patches somewhere?)

(In reply to comment #38) > has any work been done to make any of this threadsafe (like in pending patches > somewhere?) No, but using mutexes doesn't get us away from the lock contention that we want to prevent anyway.

any advice on how you want me to do this?

Comment on attachment 485904 [details] [diff] [review] interface changes v.2 >diff --git a/toolkit/components/places/public/nsINavBookmarksService.idl b/toolkit/components/places/public/nsINavBookmarksService.idl >@@ -246,16 +245,22 @@ interface nsINavBookmarksService : nsISu > * @param aTitle > * The title for the new bookmark > * @return The ID of the newly-created bookmark. > */ > long long insertBookmark(in long long aParentId, in nsIURI aURI, > in long aIndex, in AUTF8String aTitle); > > /** >+ * The Async version of insertBookmark >+ */ >+ void insertBookmarkAsync(in long long aParentId, in nsIURI aURI, >+ in long aIndex, in AUTF8String aTitle); I'd add a note regarding reasoning behind this, so that it can be used to add a bunch of bookmarks and the process happens in a background thread so that it does not hang the UI. Also a note regarding the fact this fires the common notifications through nsINavBookmarksObserver. As a side note, we must make this method wise enough to handle batching, so if this async thing is called while a bookmarks batch is active (ideally also when a history batch is active, not sure why someone bothered to create both in the 2 services), it does create a wrapping transaction around all the additions. The problem we could have here is that we cannot use runInBatchMode because it's completely sync and will commit before any statement is complete. From my experience a transaction improves quite enough I/O on Windows when dealing with this stuff (something like moving from 10s to 0.5s to add 100 bookmarks, due to fsyncs), so just having async insert bookmarks is not enough if we can't ensure multiple of them can run in a transaction. Most likely here should add a note to call this in batching when adding more than 1 bookmark at a time, but we have to define how it will work. >diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp >+NS_IMETHODIMP >+nsNavBookmarks::InsertBookmarkAsync(PRInt64 aFolder, >+ nsIURI* aURI, >+ PRInt32 aIndex, >+ const nsACString& aTitle) >+{ >+ return NS_ERROR_NOT_AVAILABLE; >+} well should be NS_ERROR_NOT_IMPLEMENTED but it probably doesn't matter.

Comment on attachment 485905 [details] [diff] [review] tests >diff --git a/toolkit/components/places/tests/bookmarks/test_519514_async_insert.js b/toolkit/components/places/tests/bookmarks/test_519514_async_insert.js put in the name the api name so test_519514_insertBookmarkAsync.js (or you can avoid the bug number and put a comment in the top part of the test with link to the bug). >+/* ***** BEGIN LICENSE BLOCK ***** ... >+ * ***** END LICENSE BLOCK ***** */ use public domain header in tests please (yes, it is allowed in tests): /* Any copyright is dedicated to the Public Domain. * http://creativecommons.org/publicdomain/zero/1.0/ */ >+let bs = PlacesUtils.bookmarks; nit: Could you directly use PlacesUtils.bookmarks everywhere? >+var observer = { >+ onBeginUpdateBatch: function() {throw "a"}, "a"? >+ onEndUpdateBatch: function() {}, >+ onItemAdded: function(id, folder, index, itemType) { >+ do_check_eq(bs.getBookmarkURI(id).spec, "http://google.com/"); >+ do_test_finished(); you should remove the observer before finishing the test >+ }, >+ onBeforeItemRemoved: function() {}, >+ onItemRemoved: function() {}, >+ _Itemchangedpropert: null, what is this for? >+ onItemChanged: function(id, property, isAnnotationProperty, value, >+ lastModified, itemType) { >+ }, >+ onItemVisited: function() { >+ }, >+ onItemMoved: function() {}, consistent bracing please, so either in one line (preferred) or 2 lines >+ QueryInterface: function(iid) { >+ if (iid.equals(Ci.nsINavBookmarkObserver) || >+ iid.equals(Ci.nsISupports)) { >+ return this; >+ } >+ throw Cr.NS_ERROR_NO_INTERFACE; >+ } XPCOMUtils is already in your scope, just use it :) >+ >+// main we don't put these main comments anymore >+function run_test() { >+ >+ bs.addObserver(observer, false); don't skype empty lines, instead follow the global code style putting the brace in its own lines when starting a function >+ >+ let root = bs.bookmarksMenuFolder; >+ >+ let testRoot = bs.createFolder(root, "places bookmarks xpcshell tests", >+ bs.DEFAULT_INDEX); >+ let testFolder = bs.createFolder(testRoot, "test Folder", bs.DEFAULT_INDEX); avoid all of this boilerplate root/folders creation and just add a bookmark to PlacesUtils.unfiledBookmarksFolderId >+ bs.insertBookmarkAsync(testFolder, uri("http://google.com/"), >+ bs.DEFAULT_INDEX, ""); >+ do_test_pending(); >+} We will also need a test for batching once we define how it should work

what we could do for batching is to always add to a sort of a queue, when the first addition starts it will init a transaction, when it ends if the queue is empty will commit it, otherwise will let next addition to close it and so on. So insertBookmarkAsync adds to the queue and starts running over it, when each run starts picks at the top of the queue, if needed starts a transaction, when the queue is found empty the transaction is committed. Could this work? This way we would also avoid having users to manually handle batching.

there is another fancy possibility similar to what we did with temp tables in history. the API could add to a temp table that would be identical to moz_bookmarks and then invoke a runnable. Each runnable would check if the bookmark is already in moz_bookmarks (we need to do this already to catch the id), if not it will call a DELETE FROM moz_bookmarks_temp; that would cause all items to move to moz_bookmarks through a BEFORE DELETE trigger (inserting NULL ids though). Finally the runnable would notify. This should be the best from a IO point of view, but we have to pay a cost to create the temp table and the trigger, on the other side we pay a queue cost in the other case as well. The first runnable would also have to select twice from moz_bookmarks, first to check if item has been synced already, second to get the id after the sync. Others would just pay once.

notice solution 2 does not require any transaction management, the addition to the temp table is all in memory, so it can even be sync, the delete and trigger are already magically put in a transaction by SQLite.

Mak, reassigning. Clearly you know exactly how to implement this.

Marco has a large number of blockers on his plate right now as it is, best to keep this unassigned in case someone else wants to come along and implement it.

(In reply to comment #40) > any advice on how you want me to do this? comment 20, third paragraph is a good starting point (In reply to comment #41) > As a side note, we must make this method wise enough to handle batching, so if > this async thing is called while a bookmarks batch is active (ideally also when > a history batch is active, not sure why someone bothered to create both in the > 2 services), it does create a wrapping transaction around all the additions. > The problem we could have here is that we cannot use runInBatchMode because > it's completely sync and will commit before any statement is complete. > From my experience a transaction improves quite enough I/O on Windows when > dealing with this stuff (something like moving from 10s to 0.5s to add 100 > bookmarks, due to fsyncs), so just having async insert bookmarks is not enough > if we can't ensure multiple of them can run in a transaction. > Most likely here should add a note to call this in batching when adding more > than 1 bookmark at a time, but we have to define how it will work. I think that making the API do something different when in a batch mode is going to lead to confusion and strange end cases, not to mention a complicated back-end for us that will be fun to test. Let's take a step back here for a second and examine our use cases: 1) Fennec add bookmark UI 2) Sync adding a bunch of bookmarks on first sync 3) Importing of bookmarks on first run (or new profile) I think that (1) and [(2) and (3)] are two distinct groups that we should really have two different APIs for. For (1), what I proposed in comment 20 and implemented in attachment 485904 [details] [diff] [review]. For (2) and (3), we need a batched API, so something like this: void insertBookmarksAsync( [array, size_is(aNum)] in nsIPlacesBookmarkData aBookmarkData, in long aNum ); interface nsIPlacesBookmarkData : nsISupports { readonly long long parentId; readonly nsIURI uri; readonly long index; readonly AUTF8String title; } The code in the backend can be shared for both of these pretty trivially I think. The only question I think we need to answer is do we notify about a batch starting and ending for the second API case? I think yes, but I'd like to hear other thoughts as well. I think we also need to change the bookmark observer API so that we also include the URI when we notify about a new item. Filing a new bug for that right now, since that needs to block beta 7.

Notice sync has also to add separators and folders... thus I think we want something more like createItemAsync accepting an itemType rather then duplicating all insertbookmark, insertSeparator, createFolder APIs with async ones. We have to send batching notifications for the UI, we also have to correctly setup mBatchingLevel, since a result created during a batch must know if a batch is running or not on creation.

(In reply to comment #49) > Notice sync has also to add separators and folders... thus I think we want > something more like createItemAsync accepting an itemType rather then > duplicating all insertbookmark, insertSeparator, createFolder APIs with async > ones. OK, let's make this bug specifically about the async add bookmark API since it was originally filed as "bookmarking on fennec is slow". Can one of the sync folks file a bug for the API they need detailing all the nsINavBookmarkService methods they use please?

Shawn - Can you assign this to someone?

(In reply to comment #51) > Shawn - Can you assign this to someone? As stated in comment 47, someone on the places team will pick it up when we have cycles to actually work on it, but until then we are going to leave it unassigned so if someone else wants to come around and work on it, they can.

shawn, no one can work on it until the design and work is outlined by you or Mak.

(In reply to comment #53) > shawn, no one can work on it until the design and work is outlined by you or > Mak. Per comment 50, we are going to go with what was outlined in comment 20 with no callback. We can rely on bookmark observers to do the right thing since bug 607309 has been fixed.

question: does fennec also support tags and keywords on bookmarks?

(In reply to comment #55) > question: does fennec also support tags and keywords on bookmarks? Fennec has full UI support for tags. It does not have a UI for editing keywords, but it has read-only support (it reads and uses keywords from synced bookmarks).

Sorry for the questions! Does it support undo/redo of bookmark creation? does it plan to support it or any other bookmarking feature that today is not yet used?

(In reply to comment #57) > Sorry for the questions! Does it support undo/redo of bookmark creation? The Fennec UI does not support undo/redo of bookmark creation. (It displays a "Remove" button in the "Page bookmarked" popup, but I believe this just does a normal deletion rather than "undo".) > does it plan to support it or any other bookmarking feature that today is not > yet used? As far as I know we do not have any plans to add more bookmarking features in the 4.0 timeline. Adding Madhava to correct me if I'm wrong.

no more features for 4.0.

stuart, this task needs ownership.

Shawn and Marco are currently tasked on other Places performance changes for Fx4 (which mobile will also benefit from!). This bug is a final+ blocker, so it's not going off the radar. They'll get to it when it is possible for them to do so. Until then, Marco can you provide the design/architecture requirements that Doug asked for, so that if someone else can pick it up, they can reasonably do so without wasting effort on the wrong approach?

Nm, design is in comment #54.

I think I'm going to finish work on my last betaN blocker today (excluding a couple that are blocked on something that is not my duty), on Monday I'd like to do a global check of the situation for async bookmarks. We have 2 clear use-cases: Fennec (add a single bookmark, then tags) and Sync (add a bunch of bookmarks with tags, keywords and whatever). In both cases tags and keywords (or anything other that could be set on the bookmark synchronously) could be a problem if done separately from this async API. Suppose I add a bookmark (asynchronously) and immediately try to tag it (synchronously). Fennec can probably just wait the bookmark observer before allowing to edit the bookmark itself. If API addition would be an issue we can just make a asyncBookmarks.jsm and add a notifying API (APIs like nsPIPlacesXXX are never frozen for us, since they are intended for our internal use). If we could come up with a good batch addition API, fennec could even just use it with N=1.

It sounds like he design in comment #54 isn't something should work on since Marco is going to investigate next week.

Marco, any update?

the design for async API is on the wiki https://wiki.mozilla.org/index.php?title=Places/AsyncAPIsForSync Mobile can use the same API we'll be adding for Sync, the branch merge is at its end, and this async API is the first priority after the merge. Also, with current work on the branch it's possible the situation for Mobile will get a bit better even before we have the full API.

(In reply to comment #66) > the design for async API is on the wiki > https://wiki.mozilla.org/index.php?title=Places/AsyncAPIsForSync Mobile can use > the same API we'll be adding for Sync, the branch merge is at its end, and this > async API is the first priority after the merge. Also, with current work on the > branch it's possible the situation for Mobile will get a bit better even before > we have the full API. Hold up! That hasn't gotten a final stamp of approval. I thought mobile was interested in getting History done first since that was the bigger problem? Getting the final stamp of approval hasn't been a priority because of that.

As the vast bulk of the performance impact from places+sync is tied to history (covered by bug 606966), not bookmarks, this should not block Fx4. The cost in terms of complexity/risk/schedule impact are not justified by the amount of work here. We should absolutely fix this as soon as possible, but doing it this late in the cycle is not necessary or desirable from a risk mitigation perspective. Minusing blocking2.0 per triage, re-nominating for Fennec blocking. bug 606966 will continue to block, and effort will be concentrated there.

we still have perf issues (see bug 618715) related to bookmarking outside of sync

I guess bug 618715 and bug 621802 made the situation much better (I was expecting us to spend a lot of time in checking if the page was bookmarked, since we were going the slow path). Is this still in a blocking status after those improvements? Right now the only slowdown should be when the user changes the bookmarked status of a page. Are there still code paths touching the database synchronously apart insertBookmark in the star code?

Minusing based on comments -- please renominate if this is still a major issue after the history pieces land.

From our SQL telemetry looks like people use bookmarks a lot and this causes significant delays on main thread. For some reason lastModified is set a lot. Are we setting lastModified every time we visit a bookmark? "sql" "count" "time" "avg_avg_time" "median_avg_time" "sd_avg_time" "avg_time" "UPDATE moz_bookmarks SET lastModified = :date WHERE id = :item_id" 6179 2023460 268.14117471257 151 711.728713127675 327.473701246156

(In reply to Taras Glek (:taras) from comment #72) > From our SQL telemetry looks like people use bookmarks a lot and this causes > significant delays on main thread. For some reason lastModified is set a > lot. Are we setting lastModified every time we visit a bookmark? For a while I had that doubt, even if that would have been crazy, I checked and no, we don't set lastModified on visit. I think this is just the effect of either livemarks (bug 613588 will solve that part) or Sync (this bug should solve this part). Regarding why that query may be slow, the only thing I may think of is that it is usually the last things we do after changing a bookmark, being the last operation in a transaction it may cause a wal checkpoint. Again this bug should solve that by moving the work to the async thread.

mak, what's the status of this? Is this bug, or Bug 628315, the right way to move forward?

The status of this is "let's first finish making history completely async", history is still the main concern due to its size. We are not that far from that target, some tests to fix, Sync to fix, then we can make history removals async and remove a bunch of legacy code. Before attacking this we also have to fix the surroundings, we make HTML import/export async ready, we should do the same with json backups. Then I think we may attack this. So, depending on resources availability, Bug 628315 may be a good first step, this one is still the right long term fix.

(In reply to Marco Bonardo [:mak] from comment #75) > The status of this is "let's first finish making history completely async", > history is still the main concern due to its size. We are not that far from > that target, some tests to fix, Sync to fix, then we can make history > removals async and remove a bunch of legacy code. Great. One of the things we want to do in the early-to-mid-20s releases is to have Sync's internal structure be entirely non-blocking (I hesitate to use the word "async" for JS!). That's the only way it'll scale to a bigger user base without pain. Fixing the current Sync codebase will involve adding explicit event loop spinning, which is unpleasant, so I'd only suggest switching if the synchronous APIs are going away. Better to wait a little while. > Before attacking this we also have to fix the surroundings, we make HTML > import/export async ready, we should do the same with json backups. Then I > think we may attack this. Are there bugs filed and tied together for all of these? I'd like to have those dependencies tracked somewhere. > So, depending on resources availability, Bug 628315 may be a good first > step, this one is still the right long term fix. Got it, thanks! Bug 628315 should be fairly straightforward to implement, particularly with a nice Promises layer on top, so we might do that for the first run at the new bookmarks engine. Keep me posted!

(In reply to Richard Newman [:rnewman] from comment #76) > Fixing the current Sync codebase will involve adding explicit event loop > spinning, which is unpleasant, so I'd only suggest switching if the > synchronous APIs are going away. Better to wait a little while. I agree. > > Before attacking this we also have to fix the surroundings, we make HTML > > import/export async ready, we should do the same with json backups. Then I > > think we may attack this. > > Are there bugs filed and tied together for all of these? I'd like to have > those dependencies tracked somewhere. the json backups bug has still to be filed (going to do that now), the global tracking bug for backup/html is bug 549736

Hey Marco, I'm converting this bug to a breakdown, for this iteration we should have this one, bug 1068007 and bug 1068009.. I will file more dependencies later.

hm, doh, better if I file a separate breakdown bug... wait a minute pls.

ok, this one is a meta, the breakdown bug is bug 1068015. so in the backlog we need bug 1068015, bug 1068007 and bug 1068009

Thanks Marco. I've removed this bug from the list and will add the breakdown. (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #80) > ok, this one is a meta, the breakdown bug is bug 1068015. > so in the backlog we need bug 1068015, bug 1068007 and bug 1068009

unassigning meta.