Closed Bug 1001467 Opened 10 years ago Closed 7 years ago

[Messages] Background deletion of messages

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: drs, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [c=progress p= s= u=])

Attachments

(2 files, 3 obsolete files)

In bug 999095, we found out that the waiting screen and not updating the UI while waiting for IPC was slowing us down pretty severely. We should background this process to not only speed it up but also make it more perceptibly fast on top of this.
Attached patch Background message deletion. (obsolete) — Splinter Review
I don't think this needs tests (bring on the Two Minutes Hate).

https://github.com/mozilla-b2g/gaia/pull/18690
Assignee: nobody → drs+bugzilla
Attachment #8412674 - Flags: review?(felash)
Blocks: 999095
Keywords: perf
Whiteboard: [c=progress p= s= u=]
> I don't think this needs tests

Famous last words

;-)
Status: NEW → ASSIGNED
Priority: -- → P2
Depends on: 1006133
Attached patch Background message deletion. (obsolete) — Splinter Review
I added some tests in bug 1006133, and this patch removes a couple of them. To be honest, I'm not sure what behavior is worth testing as a result of the changes in this patch.

Updated PR.
Attachment #8412674 - Attachment is obsolete: true
Attachment #8412674 - Flags: review?(felash)
Attachment #8417719 - Flags: review?(felash)
Forget comment 4, bad bug.
Comment on attachment 8417719 [details] [diff] [review]
Background message deletion.

You need to rebase the patch, sorry.

So, yes, it makes the deletion process noticeably faster, but in the cases where it's faster it's useful to have this screen to tell the user something's happening.

Therefore I think we need to try other things.

I can think of:
* remove the spinner
* replace the spinner with something less expensive
* find why the spinner is so expensive
* have another less expensive way to show something is happening (we need UX for this)
* see what happens if the waiting screen's background is not a non-opaque PNG.
Attachment #8417719 - Flags: review?(felash)
Attached patch Background message deletion. (obsolete) — Splinter Review
Rebased and updated PR.

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Comment on attachment 8417719 [details] [diff] [review]
> Background message deletion.
> I can think of:
> * remove the spinner
> * replace the spinner with something less expensive
> * find why the spinner is so expensive

None of these are the problem or solution. Removing the spinner entirely gave me about a 10% performance boost when I tried it, IIRC. It wasn't significant.

> * have another less expensive way to show something is happening (we need UX
> for this)
> * see what happens if the waiting screen's background is not a non-opaque
> PNG.

I'm not convinced that we need to let the user know about the deletion progress. I've found that on average, deleting an entire MMS thread, as well as all but one message in it, with a hundred messages takes me less than a second on Fugu. Even if IndexedDB/IPC locks up for some reason, the app is still usable, and you can even scroll around while reflowing/repainting since it's now async and wasn't when this was designed. If the deletion fails, then we should be presenting an error message to the user. If for some strange reason the deletion takes 10+ seconds, is it really better to keep the user stuck on the waiting screen, or is it better to just let them continue using the app even though it might be slow?

I'm in agreement with you on deleting threads, though. I think deleting threads is going to require an entirely different approach.
Attachment #8417719 - Attachment is obsolete: true
Attachment #8421138 - Flags: review?(felash)
I tried deleting all messages from a big thread (from my own data, so it's a real life's use case) and it took about 15/20 seconds without your patch, and about 7/10 seconds with your patch (very approximately). 7/10 seconds is a lot in my opinion.

I can scroll around but this is useless and quite error prone too because we can interact with things that are in the process of being deleted.

Moreover, we can encounter this bug in [1] (messageBubble is null) which admittedly needs to be fixed.

Clearly, if we go this path, we need to also exit edit mode and do as if the delete operation is finished.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L1915
(In reply to Julien Wajsberg [:julienw] from comment #8)
> I tried deleting all messages from a big thread (from my own data, so it's a
> real life's use case) and it took about 15/20 seconds without your patch,
> and about 7/10 seconds with your patch (very approximately). 7/10 seconds is
> a lot in my opinion.

Oh, interesting. No matter what thread I was working with, I couldn't find one that took longer than about 2 seconds to delete any number of messages with this patch.

Do you know how many messages you deleted? I was working with a couple hundred with images (from the reference workloads).

If it's the length of the thread that's the issue, maybe we can account for the length of the thread and either show or not show the overlay depending on how many messages the user is deleting. I'm not a fan of heuristics like this, but we can make it really conservative. For example, if you try to delete 10+ messages, it shows the overlay. What do you think of that?

> I can scroll around but this is useless and quite error prone too because we
> can interact with things that are in the process of being deleted.

Yeah, I agree. Any more than about 2-3 seconds on average and the reflowing will really get in the way.

> Moreover, we can encounter this bug in [1] (messageBubble is null) which
> admittedly needs to be fixed.

I'm not sure I follow here. I think if the user long pressed on a message bubble while it was being reflowed, it would turn it into a sync reflow if it wasn't already.
(In reply to Doug Sherk (:drs) from comment #9)
> (In reply to Julien Wajsberg [:julienw] from comment #8)
> > I tried deleting all messages from a big thread (from my own data, so it's a
> > real life's use case) and it took about 15/20 seconds without your patch,
> > and about 7/10 seconds with your patch (very approximately). 7/10 seconds is
> > a lot in my opinion.
> 
> Oh, interesting. No matter what thread I was working with, I couldn't find
> one that took longer than about 2 seconds to delete any number of messages
> with this patch.
> 
> Do you know how many messages you deleted? I was working with a couple
> hundred with images (from the reference workloads).

I can try to find out but certainly more than 1500.

> 
> If it's the length of the thread that's the issue, maybe we can account for
> the length of the thread and either show or not show the overlay depending
> on how many messages the user is deleting. I'm not a fan of heuristics like
> this, but we can make it really conservative. For example, if you try to
> delete 10+ messages, it shows the overlay. What do you think of that?

This could work, but please read below too :)

> 
> > I can scroll around but this is useless and quite error prone too because we
> > can interact with things that are in the process of being deleted.
> 
> Yeah, I agree. Any more than about 2-3 seconds on average and the reflowing
> will really get in the way.


So, discussing with Oleg this morning made me more open to the "let's remove in the UI and delete in background" possibility, especially in the "delete messages in a thread" case where we have only one "delete" API call. I'm still not sure for the "delete threads" case.

Obviously we would not display the waiting screen in that case. From your profile, do you think this would make things look good?

> 
> > Moreover, we can encounter this bug in [1] (messageBubble is null) which
> > admittedly needs to be fixed.
> 
> I'm not sure I follow here. I think if the user long pressed on a message
> bubble while it was being reflowed, it would turn it into a sync reflow if
> it wasn't already.

Yep you're right. I was saying that I saw this bug happening but probably more because of the reflow than because of the fact we were deleting in background.
(In reply to Julien Wajsberg [:julienw] from comment #10)
> So, discussing with Oleg this morning made me more open to the "let's remove
> in the UI and delete in background" possibility, especially in the "delete
> messages in a thread" case where we have only one "delete" API call. I'm
> still not sure for the "delete threads" case.
> 
> Obviously we would not display the waiting screen in that case. From your
> profile, do you think this would make things look good?

Sure, here's a profile with the patch that I attached in this post (updated PR too):
http://people.mozilla.org/~bgirard/cleopatra/#report=7d1c850a866e597e8e2739fb81c521fa30325b0e

Without it:
http://people.mozilla.org/~bgirard/cleopatra/#report=d07a4779466ab29e4d343eb00ec8e4b08572d4a2

You'll notice that there's a block of displaylist-rasterize missing from the profile with the patch, which covers about 750ms. The post-delete reflow/repaint process isn't any faster, but it feels snappier without the overlay. In total, it looks to me like we're going from ~3s->~2.25s, plus the added "feeling" of responsiveness.

Based on this, do you agree with my idea in comment 9? Just to be clear, this patch does nothing to the thread deletion process. That is, it'll still show the progress overlay for thread deletion. If you agree, what do you think is a reasonable number of messages being deleted to show the overlay for? Maybe 100?

Also, slightly off topic: I started looking into adding a "deleteThread" API call to the mobile message DB, but I realized that Oleg is working on switching us to a Gaia-side datastore in bug 898364. Knowing the performance expectations there would be really helpful in deciding this. Oleg, can you provide any info on this?
Attachment #8421138 - Attachment is obsolete: true
Attachment #8421138 - Flags: review?(felash)
Attachment #8421918 - Flags: review?(felash)
Flags: needinfo?(azasypkin)
Here it is with the 100+ message heuristic. Please review whichever one you like more.
(In reply to Doug Sherk (:drs) from comment #11)
> Also, slightly off topic: I started looking into adding a "deleteThread" API
> call to the mobile message DB, but I realized that Oleg is working on
> switching us to a Gaia-side datastore in bug 898364. Knowing the performance
> expectations there would be really helpful in deciding this. Oleg, can you
> provide any info on this?

Hey Doug, I'm afraid that there is too much uncertainty here. It's still not clear what part of API Gecko will be responsible for. As per [1] it seems that the majority of "write" operations will be left in Gecko anyway. Also it seems that Datastore is going to change and become more like a shared IDB [2].

Regarding to adding "deleteThread" API method to message DB, I think it would be beneficial in any case. Even if Tread-related functionality is moved to Gaia (I think there is a plan for that) we can definitely reuse almost the same code in Gaia.

[1] https://groups.google.com/forum/#!topic/mozilla.dev.webapi/7fSvp7Dklwk
[2] https://etherpad.mozilla.org/datastore
Flags: needinfo?(azasypkin)
Doug, from the profile, I don't understand why one DOM operation is taking so much time (the display list and composite operations).

:BenWa told me that if you do a profile with Gecko HEAD, we'll know why a Flush Style operation is triggered. Could you maybe try to do a new profile again with a very fresh Gecko?

Another idea that I had discussing with colleagues: maybe we can do a fast path for the "delete all" operation. I have no clue what the user is usually doing, but my guess is that he'll either delete 1 or 2 messages or delete all messages (until we have something like "delete all messages older than 7 days" that I hope will eventually come). In the "delete all" case, we can just move to the thread list (that will delete the content using `innerHTML = ''`) and launch the delete operation in background.

In the patch, I would call MessageManager.deleteMessages last (after cancelEdit).


Also, discussing with a colleague again, he told he he usually delete messages one by one; in that case we might want to stay in the edit mode instead of going out...  but that's a discussion we already had with Omega :(


So yeah, first step, another profile with a fresh Gecko that will tell us why we flush the style.
Flags: needinfo?(drs+bugzilla)
Doug, another thought: we have 2 last-child selectors here. Can you try to remove them and see if this improves something? I wonder if the last-child selectors are triggering the styles flush operation when we remove the threads.
(In reply to Julien Wajsberg [:julienw] from comment #15)
> Doug, another thought: we have 2 last-child selectors here. Can you try to
> remove them and see if this improves something? I wonder if the last-child
> selectors are triggering the styles flush operation when we remove the
> threads.

This was correct. I filed bug 1014178 and a posted a fix for it. Though unfortunately it doesn't solve the original problem here, it does help mitigate it a bit.
Depends on: 1014178
(In reply to Julien Wajsberg [:julienw] from comment #14)
> :BenWa told me that if you do a profile with Gecko HEAD, we'll know why a
> Flush Style operation is triggered. Could you maybe try to do a new profile
> again with a very fresh Gecko?

Just a note that even with the patch in bug 1014178, we're still getting those weird batches of composites for almost a second after reflowing and repainting is done after deleting message(s). I'm still not sure why -- the additional stack trace wasn't helpful, as all it said was that it was the result of the driver tick (probably some incremental work).

> Another idea that I had discussing with colleagues: maybe we can do a fast
> path for the "delete all" operation. I have no clue what the user is usually
> doing, but my guess is that he'll either delete 1 or 2 messages or delete
> all messages (until we have something like "delete all messages older than 7
> days" that I hope will eventually come). In the "delete all" case, we can
> just move to the thread list (that will delete the content using `innerHTML
> = ''`) and launch the delete operation in background.

I tried a patch that does this and the performance differences were negligible. It was actually faster without this, somehow, though I'm sure this is just due to testing with a different thread or something. 

Here's a profile with the innerHTML hotpath:
http://people.mozilla.org/~bgirard/cleopatra/#report=deed950fb2f9575ac3adb86564b675615f3bc683
Here's one without it:
http://people.mozilla.org/~bgirard/cleopatra/#report=30972ecd24e8a491c06599b70fa98357ac15107a

> In the patch, I would call MessageManager.deleteMessages last (after
> cancelEdit).

Hmm, I'd like to hear your reasons for this, but on the surface I disagree. If dispatching the DB transaction fails due to a runtime error, would we want the UI changes to still propagate?
Flags: needinfo?(drs+bugzilla)
Assignee: drs+bugzilla → nobody
Attachment #8421918 - Flags: review?(felash)
Mass closing of Gaia::SMS bugs. End of an era :(
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: