Skip to content

Conversation

jalfd
Copy link

@jalfd jalfd commented Aug 16, 2019

Fix two inefficiencies in how and when we compact the websocket receive buffer.
In most browsers, the impact of this is negligible, but on a high-res monitor in IE11, these issues could freeze the buffer for over half a second when a large FBU is received.

With these changes, compactions effectively disappear from IE11's profiler graph.

@jalfd jalfd force-pushed the optimize-receive-buffer branch from fe01fcf to df50d34 Compare August 16, 2019 17:27
Jesper Alf Dam added 2 commits August 16, 2019 19:34
Previously, we would compact the buffer (moving unread data to the
start of the buffer) as follows:

- after processing a message, if there are zero unread bytes, just reset
  the indices for first and last unread byte to zero
- else, if at least 1/8th of the buffer is used, copy remaining data to the beginning of the buffer

The second option is never actually necessary, as before inserting new data
into the array, we already check if there's enough free space, and
compact the buffer first if necessary. So we've been doing a lot of
copies that weren't actually needed. Let's not do that any more.
When compacting the receive buffer, we should only copy the bytes
between _rQi and _rQlen (the index of the first unread byte, and the
next write position).

Previously, we copied everything for _rQi up untill the end of the
buffer.
@jalfd jalfd force-pushed the optimize-receive-buffer branch from df50d34 to 08567b0 Compare August 16, 2019 17:34
@samhed samhed added the cleanup label Sep 25, 2019
@samhed samhed added this to the v1.2.0 milestone Sep 25, 2019
@samhed samhed self-assigned this Sep 25, 2019
Copy link
Member

@samhed samhed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay.

Very nice! I have to commend you for good code, helpful comments, and great commit messages. Thank you for your work.

I've looked through the commits and tested the code in IE 11 on Windows 10 with a 4k monitor. And I can see a slight improvement, very nice.

@samhed samhed merged commit c51a77c into novnc:master Sep 25, 2019
@jalfd
Copy link
Author

jalfd commented Sep 25, 2019

Thanks for the kind words. :)

And I can see a slight improvement, very nice.

FYI, I should mention that because noVNC currently downscales the canvas to compensate for your DPI scaling, you won't see the full benefit unless you set the scaling factor in Windows to 100%. If you do that, you get a canvas matching the size of your actual desktop resolution, and then the difference in performance becomes very noticeable. That's how we became aware of the problem.

@samhed
Copy link
Member

samhed commented Sep 26, 2019

Yeah, by default the scaling factor in Windows was 200%, before testing this I changed this to 100%. I guess the client machine I tested on was a bit too powerful to notice a big difference. I never did any profiling etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants