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

RESOLVED FIXED in Firefox 3.6a1

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Kevin Driedger, Assigned: mak)

Tracking

unspecified
Firefox 3.6a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [TSnappiness])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
related: timeouts during migration in the same script. bug 490035.
(Assignee)

Comment 2

9 years ago
this makes me think we are not batching there since that code should not be hit during batches.
Whiteboard: [TSnappiness]
onItemChanged it looks like again.  Agree with comment 2 - this should only happen (and even then, maybe) if we are not batching.
(Assignee)

Comment 4

9 years ago
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
Assignee: nobody → mak77
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

9 years ago
Created attachment 384192 [details] [diff] [review]
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.
Attachment #384192 - Flags: review?(sdwilsh)
(Assignee)

Updated

9 years ago
Attachment #384192 - Flags: review?(sdwilsh)
(Assignee)

Comment 6

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

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

Comment 7

9 years ago
Created attachment 384198 [details] [diff] [review]
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.
Attachment #384192 - Attachment is obsolete: true
Attachment #384198 - Flags: review?(sdwilsh)
(Assignee)

Comment 8

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

i have some further idea to apply. new patch coming.
Attachment #384198 - Flags: review?(sdwilsh)
(Assignee)

Updated

9 years ago
Blocks: 499458
(Assignee)

Comment 9

9 years ago
Created attachment 384442 [details] [diff] [review]
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).
Attachment #384198 - Attachment is obsolete: true
Attachment #384442 - Flags: review?(sdwilsh)
(Assignee)

Updated

9 years ago
Blocks: 493577
(Assignee)

Updated

9 years ago
Blocks: 428459
(Assignee)

Updated

9 years ago
Summary: Bookmark all tabs slow causes Script unresponsive → Transactions Manager should batch child transactions (was: Bookmark all tabs slow causes Script unresponsive)
(Assignee)

Updated

9 years ago
Blocks: 427521
(Assignee)

Updated

9 years ago
Blocks: 393497
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
Attachment #384442 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 11

9 years ago
Created attachment 384589 [details] [diff] [review]
patch v1.3

addressed comments.
Attachment #384442 - Attachment is obsolete: true
(Assignee)

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/rev/09daeb71ac6d
Status: NEW → RESOLVED
Last Resolved: 9 years ago
OS: Windows Server 2003 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
(Assignee)

Updated

9 years ago
Blocks: 427606
(Assignee)

Updated

9 years ago
Blocks: 472264
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1.x?
(Assignee)

Updated

5 years ago
Flags: wanted1.9.1.x?
You need to log in before you can comment on or make changes to this bug.