search

calzoneman / sync

Node.JS Server and JavaScript/HTML Client for synchronizing online media
Other
1.47k stars 232 forks source link

execEmotes is inefficient #645

Closed deerfarce closed 7 years ago

deerfarce commented 7 years ago

execEmotes might be more efficient if a single regex was used instead of testing every single emote's regex on a message, especially if a room's emote list grows into the high hundreds. Probably the only issue I can see is enforcing a specific delimiter to find emotes (maybe one that the room's admins can define?)

Average execution times using console.time() and console.timeEnd() within execEmotes (21 messages, 666 room emotes, varying 1-4 emotes per message):

Rewritten execEmotes with single regex Old execEmotes
0.0457ms 0.6907ms

Huge relative difference, but still less than 1ms. Nonetheless, efficiency is pretty nice :feelsgood: People with really slow computers might benefit too but I'm not sure on that

calzoneman commented 7 years ago

Yes, emotes are inefficient right now. It's probably fine for a couple hundred emotes, but starts to show up in the channels that have thousands.

The plan was to make the emote list a hashtable instead of a list of regexes (with a fallback to regexes for emotes that cross word boundaries, such as those with spaces in them), which would effectively make it O(number of words in the message) — which is fixed since the message has maximum size — instead of O(number of emotes). And number of operations aside, a hashtable lookup is probably quicker than a regex.

I just haven't gotten around to prioritizing it yet.

calzoneman commented 7 years ago

I've added a more efficient implementation behind a feature flag (disabled by default). I'll test that it works as expected and then remove the old approach. This approach separates emotes by whether they contain whitespace, and uses a more efficient replacement for non-whitespace emotes.

It's available on http://beta.synchtube.me, and enabled by default there.

calzoneman commented 7 years ago

The updated code is now on live, but disabled by default. You can enable it by setting CyTube.featureFlag.efficientEmotes = true in the JS console. I'll be enabling it gradually in case there were any missed edge cases.

deerfarce commented 7 years ago

Works much faster than the original it seems, I'll be trying it out too.

calzoneman commented 7 years ago

I think this new implementation doesn't quite handle certain special characters yet (e.g. <, which gets filtered to &lt;), but I'll fix that before I roll it out to everyone.

Xaekai commented 7 years ago

Isn't this ready to be closed?

calzoneman commented 7 years ago

No, I still haven't refactored or fixed the above.

Xaekai commented 7 years ago

My users are staring to get restless that emotes with ' in their code don't work.

calzoneman commented 7 years ago

While I am planning to work on the new emote implementation soon, that's unrelated to any issues with emotes today, since it is disabled by default. So please open a separate bug report for any problems with emotes today.

calzoneman commented 7 years ago

Finally got around to building a list of emotes with special characters and crap in them to verify the hashmap implementation is working. Once this bugfix goes to live, I believe the old emote processor can be removed and this issue can finally be closed out: https://github.com/calzoneman/sync/commit/dac2e41488b815848497debfd7b0dda20919f1dd

calzoneman commented 7 years ago

This is live now.

calzoneman commented 7 years ago

Closing, will follow up to strip out the old (unused) implementation later.