1535133 - gloda indexing chat conversations with many messages is janky because each new chat message triggers re-indexing the current conversation, causing a costly forced GC to potentially happen (in the worse case)

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect, P2)

Description

[Florian Quèze [:florian]]

See this profile: https://perfht.ml/2UyxHKc

indexIMConversation is already an async function, so it should be straight forward to make it do its expensive work (which is calling MailFolder.convertMsgSnippetToPlainText for each message) in chunks during idle callbacks.

Comment 1

[Florian Quèze [:florian]]

Attachment: Bug 1535133 - make gloda index IM messages in chunks during idle slices. r=mkmelin

Comment 2

[Florian Quèze [:florian]]

The patch is fully untested, I haven't even verified that there's no syntax error in it.

Comment 3

[Wayne Mery (:wsmwk)]

The power of the profiler! :)

Can we drive this in for version 68?

And I'm curious, how does ChromeUtils.idleDispatch determine idle state?
Perhaps we could use it for the likes of Bug 347837 - Slow switching between (loading) a largely-populated folder - change to lazy db closes during idle/less aggressive closing of dbs

Comment 4

[Florian Quèze [:florian]]

(In reply to Wayne Mery (:wsmwk) from comment #3)

The power of the profiler! :)

Can we drive this in for version 68?

And I'm curious, how does ChromeUtils.idleDispatch determine idle state?

I don't know the exact implementation but it's something along the lines of "the event queue contains no user event and there are a few ms of remaining CPU time available before the next time we need to refresh the screen".

Perhaps we could use it for the likes of Bug 347837 - Slow switching between (loading) a largely-populated folder - change to lazy db closes during idle/less aggressive closing of dbs

I would want to see a profile before recommending what needs to be done, but the description looks like there's disk I/O involved If it happens on the main thread, using idleDispatch won't help much; what really would need to happen is moving the I/O off main thread.

Comment 5

[Patrick Cloke [:clokep]]

Florian, is there anything we should do about this? What are the next steps?

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Patrick Cloke [:clokep] from comment #5)

Florian, is there anything we should do about this? What are the next steps?

From reading the more recent comments in bug 1575214 I don't think there's anything more for Florian to do. I'd say we need to decide from these choices (drawn from bug 1575214 comment 8):

1. implement this patch
2. do bug 1575214 (with some experimentation)
3. do both #1 and #2

I'm sure we could get some people to test a try build for the patch, which will require assigning someone on the team to pick up the patch.
Even if we do that, I suspect we should test removing the synchronous GC in bug 1575214, because GC has made significant progress in 12 years.

What do you all think?

Comment 7

[Magnus Melin [:mkmelin]]

I think we should do both.

Comment 8

[Patrick Cloke [:clokep]]

I agree, don't think there's any reason not to do both.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ea113591641e
make gloda index IM messages in chunks during idle slices. r=mkmelin