Closed Bug 1535133 Opened 2 years ago Closed 4 months ago

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)

Categories

(Thunderbird :: Instant Messaging, defect, P2)

Unspecified
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: florian, Assigned: mkmelin)

References

Details

(Keywords: perf, Whiteboard: [has profile])

Attachments

(1 file)

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.

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

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

Keywords: perf
Whiteboard: [has profile]

(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.

Version: unspecified → 60

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

Flags: needinfo?(florian)
Blocks: 1575214
Priority: -- → P2

(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?

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(clokep)
OS: Unspecified → All
Summary: gloda indexing chat conversations with many messages is janky → 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)

I think we should do both.

Assignee: nobody → mkmelin+mozilla
Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(clokep)
No longer blocks: 1575214
See Also: → 1575214
Attachment #9050781 - Attachment description: Bug 1535133 - make gloda index IM messages in chunks during idle slices, → Bug 1535133 - make gloda index IM messages in chunks during idle slices. r=mkmelin

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ea113591641e
make gloda index IM messages in chunks during idle slices. r=mkmelin

Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 78.0
Flags: needinfo?(florian)
You need to log in before you can comment on or make changes to this bug.