The idea is to have an heuristic in results that allows them to batch automatically when needed (or at least when they think it's needed).

Please, ignore the printfs, this is just for testing purposes. I suspect I will have to fix some tests, then it may be clever land this just after the next merge (5 august), we need the whole merge timeframe for proper testing in Nightly, it's hard to predict behavioral changes off-hand. Though, if this ends up working as expected, I suspect we may shortly do bug 897954 :)

Comment on attachment 780949 [details] [diff] [review] patch v1.0 This looks fine, but considering that that the tree-view is the sole use case for observer.batching (addons included), it might make sense to do this all in treeView.js.

(In reply to Mano from comment #2) > Comment on attachment 780949 [details] [diff] [review] > patch v1.0 > > This looks fine, but considering that that the tree-view is the sole use > case for observer.batching (addons included), it might make sense to do this > all in treeView.js. For the "batching" notification yes, but there is also a special handling for Refresh() calls during batches (they are merged so that we don't execute multiple refreshes). And I'm not totally sure other views should not do something about it, IIRC we keep folder nodes alive in the menu views and they keep updating.

Some update. Obviously the change has its drawbacks. Specifically, doing a bunch of changes and then reading from the result, would read data from a previous snapshot, since the batch has not yet been closed. To address this I'm forcing batch end on first request. Now, even doing that, there's still failures I was not able to figure yet, so working on those.

So, sure, in an asynchronous environment this is an issue we must address. That said, doesn't it apply to current API just as well? I mean, there's nothing that blocks a runInBatchMode callbacks from accessing results.

(In reply to Mano from comment #5) > That said, doesn't it apply to current API just as well? I mean, there's > nothing that blocks a runInBatchMode callbacks from accessing results. yes, but you're inside a batch, so you know about that. If instead batching is transparent to you, you can't tell. Surely you may listen to batching() notification and do something, but would be really fancy code since batching may begin at any time during your code.

Strangely enough, this passes all of our tests. There's lot of magic though, so the sooner we try it in Nightly, the better is, provided you are fine with the approach. On the positive side I also found a bug through the tags liveupdate test and fixed it.

Comment on attachment 785997 [details] [diff] [review] patch v2 Review of attachment 785997 [details] [diff] [review]: ----------------------------------------------------------------- Not much too say. Thanks a lot for this work! r=mano with the following comments addressed: ::: toolkit/components/places/nsNavHistoryResult.cpp @@ +57,5 @@ > + NS_ENSURE_STATE(result); \ > + if (result->mBatchInProgress) { \ > + result->EndBatch(); \ > + } \ > + PR_END_MACRO First, I prefer an inline method here (no strong feelings though) Second, If both the writer and the reviewer were confused about when should this be called, then it deserves a nice comment explaining that. @@ +2771,5 @@ > RecursiveFindURIs(onlyOneEntry, this, spec, &matches); > > + bool skipRemovedURI = false; > + if (mRemovingURI) > + mRemovingURI->Equals(aURI, &skipRemovedURI); (void) @@ +2827,5 @@ > const nsACString& aGUID, > const nsACString& aParentGUID) > { > + bool sameURI = false; > + if (mRemovingURI && NS_SUCCEEDED(mRemovingURI->Equals(aURI, &sameURI)) && sameURI) too long. @@ +4354,5 @@ > if (mRefreshParticipants.IndexOf(aContainer) == mRefreshParticipants.NoIndex) > mRefreshParticipants.AppendElement(aContainer); > } > > +// This time is used for smart batches handling. time? timer? interval? @@ +4474,5 @@ > +nsNavHistoryResult::EndBatch() { > + MOZ_ASSERT(mBatchInProgress); > + if (mEndBatchTimer) { > + mEndBatchTimer->Cancel(); > + mEndBatchTimer = nullptr; As noted on IRC, timers are reusable.

I kept the macro for lack of time in fighting the compiler, the nodes forwards are quite fancy. We can clean this up at a later time. I added a comment explaining its usage though.

https://tbpl.mozilla.org/?tree=Try&rev=a2ebb56bb8d7 https://hg.mozilla.org/integration/mozilla-inbound/rev/2174a2ce7b02

pushed this temporary fix to skip test_nsINavHistoryViewer.js on Windows, looks like creating a bunch of bookmarks on these tinderboxes can elapse the timeout. it's not the first time we notice Windows tinderboxes being extremely slow. https://hg.mozilla.org/integration/mozilla-inbound/rev/e445650b4202 I will file a separate bug to investigate and properly address this.

https://hg.mozilla.org/mozilla-central/rev/2174a2ce7b02 https://hg.mozilla.org/mozilla-central/rev/e445650b4202

(In reply to Marco Bonardo [:mak] from bug 913160 comment #14) > Comment on attachment 813998 [details] [diff] [review] > patch v1 (-w) > > After discussing on IRC, we will backout bug 894331 from branches and > prepare a different solution for trunk. Reopening for backout (this wouldn't apply cleanly, so leaving for you to do).

we are not going to backout this on trunk, so I don't think this should be reopened, I will attach backout patches to bug 913160 only for Aurora/Beta and file a separate bug for trunk that will build on top of this change but will touch different code.

(In reply to Marco Bonardo [:mak] from comment #14) > we are not going to backout this on trunk Oh. This bug is quite frustrating on trunk - and I'm slightly wary about using a newer release to modify my places DB :-(

backed out until we have a proper solution to the hangs ready. https://hg.mozilla.org/integration/mozilla-inbound/rev/913739c56e72

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/913739c56e72

This is unlikely to be what we need long term, we should rather do bug 1340498.