related: timeouts during migration in the same script. bug 490035.
this makes me think we are not batching there since that code should not be hit during batches.
onItemChanged it looks like again. Agree with comment 2 - this should only happen (and even then, maybe) if we are not batching.
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
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.
Comment on attachment 384192 [details] [diff] [review] patch v1.0 ugh, probably there's the same issue in placesRemoveItemTransaction, clearing review request for now.
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.
Comment on attachment 384198 [details] [diff] [review] patch v1.1 i have some further idea to apply. new patch coming.
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).
Summary: Bookmark all tabs slow causes Script unresponsive → Transactions Manager should batch child transactions (was: Bookmark all tabs slow causes Script unresponsive)
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+
Created attachment 384589 [details] [diff] [review] patch v1.3 addressed comments.
Attachment #384442 - Attachment is obsolete: true
Status: NEW → RESOLVED
Last Resolved: 9 years ago
OS: Windows Server 2003 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
You need to log in before you can comment on or make changes to this bug.