Closed Bug 894331 Opened 6 years ago Closed 3 years ago

Automatic batches in Places results

Categories

(Toolkit :: Places, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mak, Assigned: mak)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [Async] p=8)

Attachments

(1 file, 2 obsolete files)

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).
Blocks: 897954
Attached patch patch v1.0 (obsolete) — Splinter Review
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 :)
Attachment #780949 - Flags: feedback?(mano)
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.
Attachment #780949 - Flags: feedback?(mano) → feedback+
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #780949 - Attachment is obsolete: true
Attachment #785997 - Flags: review?(mano)
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.
Attachment #785997 - Flags: review?(mano) → review+
Summary: Investigate whether we can overcome the concept of batches in results → Automatic batches in Places results
Attached patch patch v3Splinter Review
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.
Attachment #785997 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=a2ebb56bb8d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2174a2ce7b02
Flags: in-testsuite+
Keywords: perf
Whiteboard: [Async]
Target Milestone: --- → mozilla26
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.
Depends on: 901907
Depends on: 902765
Depends on: 902248
Depends on: 913160
Depends on: 914294
(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).
Status: RESOLVED → REOPENED
Flags: needinfo?(mak77)
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(mak77)
Resolution: --- → FIXED
(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
Status: RESOLVED → REOPENED
Depends on: 937560
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
Whiteboard: [Async] → [Async] p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: [Async] p=0 → [Async] p=8
Priority: -- → P3
This is unlikely to be what we need long term, we should rather do bug 1340498.
Status: REOPENED → RESOLVED
Closed: 6 years ago3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.