Madhava Enros [:madhava] Reporter
	Description • 15 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 1 • 15 years ago

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]

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 2 • 15 years ago

I guess that's probably related to bug 515757.

	Aakash Desai [:aakashd]
	Comment 3 • 15 years ago

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.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 4 • 15 years ago

(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.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 5 • 15 years ago

(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.

	Aakash Desai [:aakashd]
	Comment 6 • 15 years ago

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

	Madhava Enros [:madhava] Reporter
	Comment 7 • 15 years ago

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.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 8 • 15 years ago

(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?

	Madhava Enros [:madhava] Reporter
	Comment 9 • 15 years ago

Ah, good question.  Let me try.

Nope -- same problem with a fresh profile.

	Benjamin Stover (:stechz)
	Comment 10 • 15 years ago

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.

	Aakash Desai [:aakashd]
	Comment 12 • 14 years ago

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

	Benjamin Stover (:stechz)
	Comment 13 • 14 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 14 • 14 years ago

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

	Benjamin Stover (:stechz)
	Comment 15 • 14 years ago

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?

	Dariusz Filipski
	Comment 16 • 14 years ago

Adding myself to CC to track the progress.

	Shawn Wilsher :sdwilsh
	Comment 17 • 14 years ago

(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.

	Shawn Wilsher :sdwilsh
	Comment 18 • 14 years ago

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

	Mike Connor [:mconnor]
	Comment 19 • 14 years ago

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

	Shawn Wilsher :sdwilsh
	Comment 20 • 14 years ago

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.

	Shawn Wilsher :sdwilsh
	Comment 21 • 14 years ago

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);
}

	Shawn Wilsher :sdwilsh
	Comment 22 • 14 years ago

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)

	Marco Bonardo [:mak]
	Comment 24 • 14 years ago

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?

	Shawn Wilsher :sdwilsh
	Comment 25 • 14 years ago

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.

	Shawn Wilsher :sdwilsh
	Comment 26 • 14 years ago

(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).

	Marco Bonardo [:mak]
	Comment 27 • 14 years ago

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

	Doug Turner (:dougt)
	Comment 28 • 14 years ago

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

	Shawn Wilsher :sdwilsh
	Comment 32 • 14 years ago

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

	Doug Turner (:dougt)
	Comment 33 • 14 years ago

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

	Philipp von Weitershausen [:philikon]
	Comment 34 • 14 years ago

(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.

	Shawn Wilsher :sdwilsh
	Comment 35 • 14 years ago

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.

	Doug Turner (:dougt)
	Comment 36 • 14 years ago

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?

	Shawn Wilsher :sdwilsh
	Comment 37 • 14 years ago

(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.

	Doug Turner (:dougt)
	Comment 38 • 14 years ago

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?)

	Shawn Wilsher :sdwilsh
	Comment 39 • 14 years ago

(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.

	Doug Turner (:dougt)
	Comment 40 • 14 years ago

any advice on how you want me to do this?

	Marco Bonardo [:mak]
	Comment 41 • 14 years ago

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.

	Marco Bonardo [:mak]
	Comment 42 • 14 years ago

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

	Marco Bonardo [:mak]
	Comment 43 • 14 years ago

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.

	Marco Bonardo [:mak]
	Comment 44 • 14 years ago

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.

	Marco Bonardo [:mak]
	Comment 45 • 14 years ago

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.

	Doug Turner (:dougt)
	Comment 46 • 14 years ago

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

	Shawn Wilsher :sdwilsh
	Comment 47 • 14 years ago

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.

	Shawn Wilsher :sdwilsh
	Comment 48 • 14 years ago

(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.

	Marco Bonardo [:mak]
	Comment 49 • 14 years ago

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.

	Shawn Wilsher :sdwilsh
	Comment 50 • 14 years ago

(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?

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 51 • 14 years ago

Shawn - Can you assign this to someone?

	Shawn Wilsher :sdwilsh
	Comment 52 • 14 years ago

(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.

	Doug Turner (:dougt)
	Comment 53 • 14 years ago

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

	Shawn Wilsher :sdwilsh
	Comment 54 • 14 years ago

(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.

	Marco Bonardo [:mak]
	Comment 55 • 14 years ago

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

	Matt Brubeck (:mbrubeck)
	Comment 56 • 14 years ago

(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).

	Marco Bonardo [:mak]
	Comment 57 • 14 years ago

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?

	Matt Brubeck (:mbrubeck)
	Comment 58 • 14 years ago

(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.

	Doug Turner (:dougt)
	Comment 59 • 14 years ago

no more features for 4.0.

	Doug Turner (:dougt)
	Comment 60 • 14 years ago

stuart, this task needs ownership.

	Dietrich Ayala (:dietrich)
	Comment 61 • 14 years ago

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?

	Dietrich Ayala (:dietrich)
	Comment 62 • 14 years ago

Nm, design is in comment #54.

	Marco Bonardo [:mak]
	Comment 63 • 14 years ago

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.

	Doug Turner (:dougt)
	Comment 64 • 14 years ago

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

	Brad Lassey [:blassey] (use needinfo?)
	Comment 65 • 14 years ago

Marco, any update?

	Marco Bonardo [:mak]
	Comment 66 • 14 years ago

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.

	Shawn Wilsher :sdwilsh
	Comment 67 • 14 years ago

(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.

	Mike Connor [:mconnor]
	Comment 68 • 14 years ago

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.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 69 • 14 years ago

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

	Marco Bonardo [:mak]
	Comment 70 • 14 years ago

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?

	Stuart Parmenter
	Comment 71 • 14 years ago

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

	(dormant account)
	Comment 72 • 13 years ago

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

	Marco Bonardo [:mak]
	Comment 73 • 13 years ago

(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.

	Richard Newman [:rnewman]
	Comment 74 • 12 years ago

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

	Marco Bonardo [:mak]
	Comment 75 • 12 years ago

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.

	Richard Newman [:rnewman]
	Comment 76 • 12 years ago

(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!

	Marco Bonardo [:mak]
	Comment 77 • 12 years ago

(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

	Marco Bonardo [:mak]
	Comment 78 • 10 years ago

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.

	Marco Bonardo [:mak]
	Comment 79 • 10 years ago

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

	Marco Bonardo [:mak]
	Comment 80 • 10 years ago

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

	Marco Mucci [:MarcoM]
	Comment 81 • 10 years ago

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

	Marco Bonardo [:mak]
	Comment 82 • 10 years ago

unassigning meta.