896201 - Don't use "child transactions" in places

Status: RESOLVED INVALID

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

nsITransactionManager has this "implicit batch" behavior which works like this: If nsITransactionManager.doTransaction it calls nsITransaction.doTransaction and that calls back nsITransactionManager.doTransaction with another transactions, then the transactions are batched together. While the underlying structure is a little bit different (batches can be nested and all that jazz), this implicit batching isn't different then calling nsITransactionManager.beginBatch before executing the first transaction and nsITransactionManager.endBatch after the last transaction is executed.

Now, the new asynchronous transaction manager won't - and can't - work this way. Won't, because I'm aiming not to diverge too much from the semantics of the HTML 5 Undo Manager, which simply disallows "execute" (nsITransaction.doTransaction) call transact (nsITransactionManager.doTransaction); Can't - because transactions are going to be asynchronous. I'm also not likely to implement nested batches.

The way to batch in the new module is going to be something like this:

PlacesTransactions.spawn(function(aUndoManager) {
  let createFolder = PlacesTransactions.CreateFolder(...);
  let folderId = yield aUndoManager.transact(creaeFolder);
  for (...) {
    let createBookmark = PlacesTransaction.CreateBookmark(...folderId);
    yield UndoManager.transact(createBookmark, true /* aMerge */);
  }
});

This is nothing like child transactions, but it does resemble batching in a way (avoiding the downside of the need to explicitly call endBatch), albeit this batching is "flat" (no nesting).

Unfortunately, the current implementation of places transaction makes quite a heavy use of child transactions. Thus, fixing consumers isn't going to be like Find & Replace. We'll have to do away with the child transactions.

So, in order to limit the scope of the patch on bug 891303, I'm going to fix all these child transactions usage (replacing it with explicit batching) in the current system ahead of fixing the consumers to use the new API. Not that I like working twice as much, but this is going to simplify the work for bug 891303 a lot, and also reduce the potential for regressions.

Comment 1

[Asaf Romano (gone)]

One nasty problem though is that PlacesCreateFolderTransaction also takes care of using runInBatchMode (not to be confused with transactions batching!): if the number of transactions to execute is greater than five, than it runs doTransaction in batching . It would be quite irritating to do this check in every consumer, so, I'm going to make this bug dependent on Marco's bug 894331.

I could add a temporary utility in PlacesUtils, of course. It would probably hit Aurora, but I don't expect it to get into Beta.

_______
Oh, and I just figured out PlacesUtils doesn't actually use the child transactions feature of the transaction manager, but rather takes the approach of reimplementing for absolutely no apparent reason. This really couldn't get worse.

Comment 2

[Asaf Romano (gone)]

Attachment: bookmarkProperties

This code... wow, just wow, and I'm afraid I wrote it. Look at that call to "_getDescriptionAnnotation" for example.

Anyway, that's the easy part. The real work involves PlacesUIUtils.makeTransaction.

Comment 3

[Asaf Romano (gone)]

Just to be clear, this code is going to be very short lived. The goal here is to make bug 894331 a little bit more manageable.

Comment 4

[Asaf Romano (gone)]

Thinking further, I've a much more elegant solution for these cases. makeTransaction, for instance, could be getTransactionTask: a function that returns a Task to be either passed to PlacesTransactions.spawn by its caller, or, if the caller is a transaction-spawned task by itself, just called (well, yielded) with the undo manager instance it has.

Or, in the case addressed in the attached patch, createNewItem would be getCreateItemTask.

Fortunately, this is a pretty nice solution for all these cases. Unfortunately I cannot do this prepare-the-ground work.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Mano from comment #1)
> It would be quite irritating to do
> this check in every consumer, so, I'm going to make this bug dependent on
> Marco's bug 894331.

that won't help with database transactions though... even if I suspect in WAL mode it doesn't matter much cause it never fsyncs until over the given checkpoint limit.

Comment 6

[Asaf Romano (gone)]

I can make PlacesTransaction.spawn *always* start a database batch (somehow.. that's for the bug 890203), but I wonder what would be the side effects (I mean, after you make views batches independent of database batches).

Alternatively I can make PlacesTransaction.spawn take a batch parameter (again, depending on your bug and bug 894331). It's not too bad, but it would be nice to avoid that.

Comment 7

[Marco Bonardo [:mak]]

we should measure the effect, as I said I don't expect transactions to be that much important in WAL mode... but I never made an actual measurement.