Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process received messages in queue #1041

Closed
wants to merge 2 commits into from
Closed

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Apr 16, 2017

Creating early PR to gather feedback. Want to hear what @thelounge/maintainers think.

Connecting to ZNC with history playback no longer freezes my browser, and appends messages in small queues instead of as-soon-as-received.

What I want to achieve is to process multiple messages for one channel at once, and append them at once (document fragments). Thus reducing DOM manipulations and style recalculations. This should also prioritise currently active channel.

  • Sort messages per channel
  • Prioritize currently active channel
    • How does this handle for PMs/whois which force a new window to appear?
  • Use document fragments to append multiple messages at once
  • Trigger events (notifications, sticky scroll) once per processed channel
    • Look more into notifications
  • "show more" is constantly checked on an interval #96 Remove 10 second interval timer to hide messages in hidden channels
  • Limit notification sound to play every 500 milliseconds
  • Fix condensed stuff

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Apr 16, 2017
@astorije astorije added the Meta: Do Not Merge This PR should not be merged. label Apr 17, 2017
@astorije
Copy link
Member

@xPaw, adding the do not merge label for now, feel free to assign folks and remove the label when you're done with it.

@xPaw xPaw force-pushed the process-messages-in-queue branch 2 times, most recently from 73a6f80 to 21b08ed Compare April 22, 2017 15:06
@xPaw xPaw changed the title Process received messages in queue by using requestAnimationFrame Process received messages in queue Apr 22, 2017
@xPaw
Copy link
Member Author

xPaw commented Apr 22, 2017

Did another optimization pass, and removed that ugly 10 second interval timer to hide messages in non visible channels (#96).

@astorije
Copy link
Member

Rebased with master. Now back to boarding a plane 😄

@astorije
Copy link
Member

Alright, so I started looking at the code (not tested yet) and I am so confused 😅.
It's some very convoluted/complex code. I'm not criticizing, I know it's for good reasons, but it makes reviewing rather difficult and pretty sure within 2 months after merging we'll be wondering why this or that change :)
Any way you could comment this code, @xPaw? I know it takes some efforts to do so, but I don't think I can fully understand the changes right now.

@xPaw
Copy link
Member Author

xPaw commented Apr 25, 2017

The main idea of this PR is to append messages to DOM on browser idle callback (giving it time to breathe), and while it might be waiting for the idle event to callback, multiple messages can be received, and if they are, as such, they are processed in a single go. This gives a very nice boost for multiple messages in a single channel, as they get appended to actual DOM with single operation.

@kode54
Copy link
Contributor

kode54 commented Apr 29, 2017

Does not work on iOS Safari:

ReferenceError: Can't find variable: requestIdleCallback

@xPaw xPaw force-pushed the process-messages-in-queue branch from 5402c28 to 7bd4b3e Compare April 30, 2017 06:59
@xPaw
Copy link
Member Author

xPaw commented Apr 30, 2017

@kode54 Fixed.

@xPaw xPaw force-pushed the process-messages-in-queue branch from 7bd4b3e to b32cb88 Compare April 30, 2017 10:13
@xPaw xPaw removed the Meta: Do Not Merge This PR should not be merged. label Apr 30, 2017
@xPaw xPaw force-pushed the process-messages-in-queue branch from ead6cd9 to 9507232 Compare April 30, 2017 10:45
@xPaw xPaw added this to the 2.4.0 milestone May 1, 2017
@xPaw xPaw force-pushed the process-messages-in-queue branch from 9507232 to 636af64 Compare May 6, 2017 14:28
@xPaw xPaw force-pushed the process-messages-in-queue branch 2 times, most recently from 484cd54 to 6abf685 Compare June 21, 2017 08:24
@xPaw xPaw force-pushed the process-messages-in-queue branch 2 times, most recently from 9d229aa to 9265832 Compare July 18, 2017 15:35
@astorije
Copy link
Member

@xPaw, I see some empty checkbox in here, do you still want to work on this? I wouldn't like to start reviewing 300+ lines of changes if you happen to still edit it :)

@xPaw
Copy link
Member Author

xPaw commented Jul 21, 2017

@astorije these checkbox might not get even done (the code is fine as is).

@xPaw
Copy link
Member Author

xPaw commented Aug 19, 2017

I have rebased this somewhat, but this certainly breaks condensed messages due to the use of document fragments.

I still want to get this eventually merged as this fixed major bottlenecks when receiving huge amount of messages (e.g. when connecting to znc and getting buffer playback).

@astorije
Copy link
Member

@xPaw, what do you think for this PR? Do you need something else done with condensed message so this PR is compatible? I understand you want your other PRs merged before taking up any more work, which is totally fair, do you want to close this PR until condensed stuff makes it possible again?
I really want this and your other work merged too, not sure how you want to proceed.

@xPaw
Copy link
Member Author

xPaw commented Aug 20, 2017

@astorije No reason to close this.

@xPaw xPaw force-pushed the process-messages-in-queue branch from 92bfa95 to dbc5db2 Compare August 20, 2017 17:21
@xPaw xPaw closed this Aug 25, 2017
@xPaw xPaw deleted the process-messages-in-queue branch September 4, 2017 17:24
@xPaw xPaw removed their assignment Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants