Closed
Bug 490742
Opened 16 years ago
Closed 16 years ago
Transactions Manager should batch child transactions (was: Bookmark all tabs slow causes Script unresponsive)
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3.6a1
People
(Reporter: linuxbox+bugzilla, Assigned: mak)
References
Details
(Whiteboard: [TSnappiness])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
related: timeouts during migration in the same script. bug 490035.
Assignee | ||
Comment 2•16 years ago
|
||
this makes me think we are not batching there since that code should not be hit during batches.
Updated•16 years ago
|
Whiteboard: [TSnappiness]
Comment 3•16 years ago
|
||
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•16 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•16 years ago
|
||
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•16 years ago
|
Attachment #384192 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 6•16 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•16 years ago
|
||
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•16 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 | ||
Comment 9•16 years ago
|
||
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•16 years ago
|
Summary: Bookmark all tabs slow causes Script unresponsive → Transactions Manager should batch child transactions (was: Bookmark all tabs slow causes Script unresponsive)
Comment 10•16 years ago
|
||
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•16 years ago
|
||
addressed comments.
Attachment #384442 -
Attachment is obsolete: true
Assignee | ||
Comment 12•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
OS: Windows Server 2003 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Assignee | ||
Updated•15 years ago
|
Flags: wanted1.9.1.x?
Assignee | ||
Updated•12 years ago
|
Flags: wanted1.9.1.x?
You need to log in
before you can comment on or make changes to this bug.
Description
•