1315479 - Use requestIdleCallback for the message handling queues

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[aleth [:aleth]]

A new DOM feature very close to what we once were considering.

Comment 1

[aleth [:aleth]]

Attachment: Use requestIdleCallback for the message display queue

Comment 2

[aleth [:aleth]]

Attachment: Use requestIdleCallback for the socket data queue

For extra smoothness, we can use it in the socket queue too.

Comment 3

[aleth [:aleth]]

Attachment: Use requestIdleCallback for the socket data queue

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8807895 [details] [diff] [review]
Use requestIdleCallback for the message display queue

Review of attachment 8807895 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice! :-)

2 questions:
- should we call cancelIdleCallback when the conversation is closed? I assume no, because the existing this._messageDisplayPending check with an early return should be enough.
- should we provide a timeout value to ensure the conversation doesn't freeze if some CPU heavy computation is happening somewhere else?

Comment 5

[aleth [:aleth]]

Attachment: Use the timeout option

(1) Right, we don't have to bother for the reason you give. (2) Here's a patch that would add a timeout of half a second. But I can't really think of any common scenario where it would be needed, so I'm not sure it's worth the extra overhead.

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8807981 [details] [diff] [review]
Use the timeout option

Review of attachment 8807981 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder how one is expected to pick the timeout value. If we add messages to the conversation only twice per second and stop each time after one message because timing.timeRemaining() is 0, it'll take ages to display a large conversation on hold.

::: chat/content/convbrowser.xml
@@ +391,5 @@
>          </body>
>        </method>
>  
> +      <!-- This is the entry point when messages are added without
> +           using appendMessage, e.g. in the log viewer. -->

that's also the scrollback when reopening a conv on hold, right?

Comment 7

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> Comment on attachment 8807981 [details] [diff] [review]
> Use the timeout option
> 
> Review of attachment 8807981 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wonder how one is expected to pick the timeout value. If we add messages
> to the conversation only twice per second and stop each time after one
> message because timing.timeRemaining() is 0, it'll take ages to display a
> large conversation on hold.

Yes, if it's the timeout that's causing the call, timing.timeRemaining() is guaranteed to be 0 because it's not an idle period. In practice I don't think the timeout case will happen unless something is wrong and there is some horrible gc/i-o/... problem. In testing, the typical idle period available seems to be between 6 and 50 ms.

So I'm still not sure if we should take the timeout addition (at the cost of an extra setTimeout per call, I guess) - thoughts?

Comment 8

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #7)

> So I'm still not sure if we should take the timeout addition (at the cost of
> an extra setTimeout per call, I guess) - thoughts?

Really not sure either, I hadn't noticed it's calling setTimeout internally.

If you want to try without the timeout, we can test it for a couple days on Nightly and see if anybody complains :).

Comment 9

[aleth [:aleth]]

Comment on attachment 8807895 [details] [diff] [review]
Use requestIdleCallback for the message display queue

Review of attachment 8807895 [details] [diff] [review]:
-----------------------------------------------------------------

OK, let's try that.

Comment 10

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/008aabedc586769dc8799b920b22c4e05dbd8fcd
Bug 1315479 - Use requestIdleCallback for the message display queue. r=florian

Comment 11

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/bccab5fc1a0636c9f57a2b03313261627fcb5f85
Bug 1315479 - Use requestIdleCallback for the socket data queue. r=clokep

Comment 12

[aleth [:aleth]]

https://hg.mozilla.org/releases/comm-aurora/rev/5bf2d86df8a0f807e68fec40989419d026d62178
Backed out changeset 008aabedc586 (Bug 1315479). rs=bustage-fix