Use requestIdleCallback for the message handling queues

RESOLVED FIXED in Instantbird 52

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

trunk
Instantbird 52
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
A new DOM feature very close to what we once were considering.
(Assignee)

Updated

3 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
For extra smoothness, we can use it in the socket queue too.
Attachment #8807896 - Flags: review?(clokep)
(Assignee)

Comment 3

3 years ago
Attachment #8807897 - Flags: review?(clokep)
(Assignee)

Updated

3 years ago
Attachment #8807896 - Attachment is obsolete: true
Attachment #8807896 - Flags: review?(clokep)
(Assignee)

Updated

3 years ago
Summary: Use requestIdleCallback for the message display queue → Use requestIdleCallback for the message handling queues
(Assignee)

Updated

3 years ago
Component: Conversation → General
Product: Instantbird → Chat Core
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?
(Assignee)

Comment 5

3 years ago
(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 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?
Attachment #8807981 - Flags: review+
(Assignee)

Comment 7

3 years ago
(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?
(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 :).
(Assignee)

Comment 9

3 years ago
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.
Attachment #8807895 - Flags: review?(florian) → review+
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/comm-central/rev/008aabedc586769dc8799b920b22c4e05dbd8fcd
Bug 1315479 - Use requestIdleCallback for the message display queue. r=florian
Attachment #8807897 - Flags: review?(clokep) → review+
(Assignee)

Comment 11

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

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 52
(Assignee)

Updated

3 years ago
Depends on: 1317743
Depends on: 1318974
You need to log in before you can comment on or make changes to this bug.