khyperia / weechat-discord

Unmaintained! And also apparently this is against their TOS so DON'T USE THIS -- Weechat plugin for Discord support - https://weechat.org/ https://discordapp.com/
MIT License
51 stars 24 forks source link

open_and_sync_buffers is way too slow for large servers #19

Open comex opened 7 years ago

comex commented 7 years ago

For every update to user/channel state, this program iterates through every channel on every server, then through every user on the server; calculates their nick, asks weechat whether that nick exists in the channel, and adds it if not.

Even if weechat were sane, this would be rather wasteful: O(n^2), assuming the rate of updates is already O(n) in the number of users. And the wasted work is not just a quick list iteration or something, but at minimum includes the allocation and string formatting involved in calculating the nick. But weechat is not sane: nicklist_nick_exists does a linear search over a linked list of nick entries, so this ends up O(n^3), and slow enough in practice to completely lock up my (admittedly puny) EC2 instance (for a Discord server in the thousands of users).

I know this is a work in progress, and you probably know already that that's an inefficient approach, but I'm filing this in case you didn't know just how inefficient :)

The code also never removes nicks, but I guess you know that already too...

khyperia commented 7 years ago

Wow. That is... honestly very impressive, I'm amazed how bad my own code is sometimes :)

The only practical reason that I'm syncing the weechat nicklist is so that autocomplete works reasonably well. As someone who is not myself (and has strange use-cases, being the dev), would you think a reasonable solution to this is to just stop syncing to the weechat userlist, and implement sane autocompletion? (so that @khy would complete @khyperia instead of having to type out khy, tab, move to the front of the nick, add the @, then continue writing). Or do you see a lot of value in having the weechat nicklist be updated?

Also, I had no idea that people other than me and one of my friends are using weechat-discord - I've just been kind of hacking away at it without much regard to usability outside of us two. So my apologies if things are a little sub-par, and thank you for the issue report!

FredrIQ commented 7 years ago

Something that seems to exacerbate this is that open_and_sync_buffers seems to be called far more than it needs to. I tweaked the nicklist update a bit to 1: clear the entire nicklist, 2: then add nicks. This didn't help, so I made it only iterate through 5 nicknames and then bail out. It ended up kind of working but still lagging badly. So I just made the event listener in event_proc.rs instead of calling open_and_sync_buffers, call nothing. This removed the lag, and allowed me to remove the 5-nick limit, but ends up not getting all the nicknames (but still more than 5). Open_and_sync_buffers is called for a reason, but perhaps some of these calls doesn't need to refresh the nicklist for every single server and channel.

EDIT: Manually adding a sync command and running it after the connection to everything is established creates a 15s lag (lurking on several servers with 2k+ people) and properly populates the nicklist. Still slow, but yeah, clearly only syncing server+channel+nicklist when actually needed is the main contender for improvement here.

khyperia commented 7 years ago

Could someone try out the latest master? I just committed https://github.com/khyperia/weechat-discord/commit/4fc82c1458f4dcffc8d5e8de9efc1c479ef08949 which likely solves this issue (and the other linked issues). Maybe. It probably also introduces a fair amount of bugs. (Honestly at this point I just want to rewrite my own discord layer instead of using discord-rs, it was a pained match to begin with and it's not getting any better)

Aanok commented 7 years ago

Loading the Discord Developers server (~5k members) takes a couple of seconds. @mentions do not autocomplete, however (regular mentions do).

Also I'm not sure if it's related since I've just installed weecord so it could come from an earlier commit, but backlogs are not getting pulled from servers.

FredrIQ commented 7 years ago

Latest master creates a large lagspike on the initial initialization (being part of several discords with 2k+ members), but seems to work OK afterwards.

khyperia commented 7 years ago

@mentions do not autocomplete, however (regular mentions do).

Yeah, this is expected :( - haven't had the time to figure out how the heck weechat's completion hooks work. It's hard to fix all these things in side projects that I only have a couple hours per week to work on (and open_and_sync_buffers was pretty high priority).

backlogs are not getting pulled from servers.

Unfortunately also expected. No code for this, as it's kind of complex (as it probably needs to be on a background thread, discord-rs blocks on log loading, which means downloading 50 lines of backlog for a couple dozen channels can take >10 seconds of weechat being completely frozen, and I'm scared to find out how long it takes for folks with hundreds of channels).

Latest master creates a large lagspike on the initial initialization

Is this a regression? I'd be worried if it is, but it's expected for weechat to freeze for a few seconds upon /discord connect - and if it went from "completely locked up forever" to "locked for a couple seconds", that's more than okay with me :)... but yeah, again, it's a crappy consequence of discord-rs doing blocking operations.

FredrIQ commented 7 years ago

Not a regression, considering that the previous behaviour was to become so slow that it was basically unusable.