490742 - Transactions Manager should batch child transactions (was: Bookmark all tabs slow causes Script unresponsive)

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Kevin Driedger]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4 (.NET CLR 3.5.30729)

If the user has a fairly large amount of bookmarks and they have never been cached the "Bookmark All Tabs" is very slow and causes the following dialog to pop up:
============
A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.

Script: file:///C:/Program%20Files/Mozilla%20Firefox%203.1%20Beta%203/components/nsPlacesDBFlush.js:241
============
Perhaps the timeout for javascript that is running the browser should be longer.


Reproducible: Didn't try

Steps to Reproduce:
1. Open the browser
2. Open about 50 tabs
3. Bookmarks->Bookmark All Tabs

Actual Results:  
Wait and see the dialog described in "Details".

Expected Results:  
Fast display of the bookmark all tabs dialog.  Perhaps if the reading and caching of the bookmarks takes too long just display a top level folder that is always available and then merge the result with the real bookmarks when the bookmarks are loaded.

Comment 1

[Jim Mathies [:jimm]]

related: timeouts during migration in the same script. bug 490035.

Comment 2

[Marco Bonardo [:mak]]

this makes me think we are not batching there since that code should not be hit during batches.

Comment 3

[Shawn Wilsher :sdwilsh]

onItemChanged it looks like again.  Agree with comment 2 - this should only happen (and even then, maybe) if we are not batching.

Comment 4

[Marco Bonardo [:mak]]

looks like we don't batch using child transactions.
This code adds a folder transaction and items inside it are created with child transactions.
We should either change code to use an aggregated transaction or make so that we will batch if child transactions are > than a certain number... i would go for the second one

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.0

i use aggregate transactions, they will automatically batch when number of transactions is over MIN_TRANSACTIONS_FOR_BATCH.
While using them i also found a bug there with do/undo/redo order, current test catched that because i used them for child transactions. So with that bug could be possible try to act on a not existant item on a redo.

I've tried to bookmark about 40 tabs at once, and it was fast enough, not instant but not boring/blocking.

We will gain more speed looking into adding methods for bookmarks service, to get/set all item's properties in a single call (useful also for syncing extensions). Or alternatively we could make a result able to represent only one item, action on nodes will be faster since a node knows everything about an item. Btw, we should have a deeper look into Transactions Manager perf. This is OT for this bug though.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 384192 [details] [diff] [review]
patch v1.0

ugh, probably there's the same issue in placesRemoveItemTransaction, clearing review request for now.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.1

handle the case all around.
Actually we use aggregateTransactions to remove bookmarks, those will batch only if we have more than 5 transactions. in case we remove a single folder we will have 1 single transaction, but it will contain a lot of child transactions. This way we will batch child transactions, that will be a good win when removing folders.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 384198 [details] [diff] [review]
patch v1.1

i have some further idea to apply. new patch coming.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.2

This tries to take in count child transactions, so that if we delete a single folder it will take in count the fact it could contain lot of children or other complex folders. And if the deletion is already inside an aggregate transaction it will try to start as soon as possible (for the external aggregate already).

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 384442 [details] [diff] [review]
patch v1.2

http://reviews.visophyte.org/r/384442/
on file: browser/components/places/src/nsPlacesTransactionsService.js line 336
>     for (var i = 0; i < aTransactions.length &&

Use let here in the loop please.
Also, can we just put each command of the for statement on it's own line. 
This is really hard to read and follow as it is currently written.


on file: browser/components/places/src/nsPlacesTransactionsService.js line 338
>       var txn = aTransactions[i].wrappedJSObject;

nit: let please


on file: browser/components/places/src/nsPlacesTransactionsService.js line 429
>       var aggregateTxn = new placesAggregateTransactions("Create folder childTxn",

nit: use let please


on file: browser/components/places/src/nsPlacesTransactionsService.js line 437
>       var aggregateTxn = new placesAggregateTransactions("Create folder childTxn",

ditto


on file: browser/components/places/src/nsPlacesTransactionsService.js line 480
>       var aggregateTxn = new placesAggregateTransactions("Create item childTxn",

ditto


on file: browser/components/places/src/nsPlacesTransactionsService.js line 633
>       PlacesUtils.bookmarks.getRemoveFolderTransaction(this._id));

I prefer to have the closing parent on a new line when you have the first
argument on a new line itself just to make it more obvious as to where it is.


on file: browser/components/places/src/nsPlacesTransactionsService.js line 656
>       var aggregateTxn = new placesAggregateTransactions("Remove item childTxn",

nit: use let


on file: browser/components/places/src/nsPlacesTransactionsService.js line 686
>       var aggregateTxn = new placesAggregateTransactions("Remove item childTxn",

nit: use let


on file: browser/components/places/src/nsPlacesTransactionsService.js line 711
>         new placesRemoveItemTransaction(contents.getChild(i).itemId));

ditto on the closing paren comment here.


r=sdwilsh with changes

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.3

addressed comments.

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/09daeb71ac6d