894331 - Automatic batches in Places results

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak]]

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).

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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 2

[Asaf Romano (gone)]

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.

Comment 3

[Marco Bonardo [:mak]]

(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.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Asaf Romano (gone)]

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.

Comment 6

[Marco Bonardo [:mak]]

(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.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v2

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 8

[Asaf Romano (gone)]

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.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v3

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.

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ed Morley [:emorley]]

(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).

Comment 14

[Marco Bonardo [:mak]]

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.

Comment 15

[Ed Morley [:emorley]]

(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 :-(

Comment 16

[Marco Bonardo [:mak]]

backed out until we have a proper solution to the hangs ready.

https://hg.mozilla.org/integration/mozilla-inbound/rev/913739c56e72

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Marco Bonardo [:mak]]

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