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
Performance degradation on 1.8.0 and newer versions #4228
Comments
Cc @seballot |
We have test coverage in for this afaik. See responsiveness frontend test. |
The problem of the reflow is easier to realize when there are a large amount of nodes. |
Oh boy, I will uncomment and ensure we have working test coverage. Hopefully @seballot can find a performant fix hastily :) |
thanks @JohnMcLear! |
Uncommented, thanks for the really thorough and well put together bug report. Will bump @seballot again 👍 |
Not heard back from @seballot yet despite email/cc here. I assume he's on vacation and will get onto this ASAP but if anyone else can take a stab while we wait that would be ideal :) |
Hi guys ! thanks for this very clear bug report @joassouza, and sorry for the bug I'm sorry I'm very focused on other project right now, I'll try to have a close look on this ticket on next week. But if anyone want to try to solve be my guest !! |
@seballot just a bump for this coming week to ensure you have some time in your schedule to take a look at this please :) Thanks! |
Hi ! I cannot reproduce, I made a 6000 lines long pad, and it works fine Tested on Debian 9 -> Chromium 73 & Firefox 68 Why your line numbers looks bad (number 1 is displayed okay, then from number 2 it's too big and not aligned) ? |
Haven't tested but can see Firefox responsive test fail that used to pass. Try Firefox responsive test with no-skin set to see if it passed? |
Yea it just passed for me too, so I need to dig deeper but I wonder if it passes for us because we're on fast(ish) pc's and sauce labs throws up some peasantry vm to handle the task? I also experience zero lag on ff on ubuntu |
Okay so changing Try doing that @seballot , I can't confirm if this is a new bug though, it would be worth checking out 1.7.9 or whatever and ensuring that the behaviour is actually different. There must be a reason the test for responsiveness was at around the 1k lines mark. It's also worth noting that the experience on Firefox 52 is the problem, modern FF seems fine? The responsiveness test crashes on FF 52 but it's fine in 80. In fact it crashes the browser. -- So I guess test in Firefox 52? I'm testing on SL now. Responsive test fails on |
Just to give more context. The tests were made on Chrome on MacOs. We could reproduce the same error on really fast machines like i9, 64gb RAM. |
I have been testing Firefox like an idiot, will test Chrome now. Okay wow yeah this is bad. I changed amount to |
I believe the delay increases if you edit in the beginning of the pad. Like, in the first line of it. |
I've tried on FF on a Windows machine and I've got the same delay. Using this pad https://video.etherpad.com/p/example_long_pad |
Hi ! indeed with 8K it start freezing. But I switched to 1.7.5 and with the pad of 8k it freeze the same (I would even say it worst !) Anyway, it's a fact that etherpad get slow when pads are too big. I don't feel it's a critical bug, because pad are not made to writes books, and I think most people use them for short text. But yes probably we could improve. But I feel it will be a painful job :) Also I'm not sure it will be only related to standard layout trashing, because one specificity is that etherpad is using different iframes, and so we always need to run javascript to align the layout between each other (for the line height for example) I would try to have a look, but I'm not promising anything because |
@JohnMcLear I tried to have a look now, but I don't understand, every change I make to |
ace2_inner.js is requested without version string atm, so the URL to ace2_inner.js does not change and your browser cache is used. When you set maxAge=0 in settings.json and/or disable browser cache it should serve the new file without restart. |
Hi @webzwo0i ! thanks for you help ! I finally remember that I need to invalidate myself the cache of ace2_inner because it's loaded in the iframe... (maxAge=0 had no impact btw) A bit late now, will look tomorrow ! |
One trick is to use network tab open as it disables caching. @joassouza can you confirm Sebastians findings? |
Due to layout trashing when calculating new heights
Ok done ! actually it was not very complicated, thanks to @joassouza who made all the work of finding both problem and solution |
and I confirm the bug is not related to recent UI changes, it has been there for a long time I think ! |
Due to layout trashing when calculating new heights
Due to layout trashing when calculating new heights
Describe the bug
Performance degradation introduced on version 1.8.0 and replicable on newer versions as well
To Reproduce
Steps to reproduce the behavior:
Expected behavior
The creation of the new line pressing ENTER should be much faster. As a baseline we can use the version 1.7.5.
Screenshots
1. The old way to deal with iframe height
This is the code that I suppose it was changing the height of the "ace2_inner" iframe on versions older than 1.8.0
https://github.com/ether/etherpad-lite/blob/1.7.5/src/static/js/ace2_inner.js#L4817
2. The new approach
As the screenshot shows there is not height style defined on the iframe. Its size is defined by the size of
#sidediv
children. This is possible because it uses flexbox.3. Video of 1.7.5 and 1.8.4 edition with the same text
https://youtu.be/LpthRFAH3GM
please, try to hear when I type
Desktop (please complete the following information):
Smartphone (please complete the following information):
I didn't test on any smartphone
Additional context
The main problem is due to the cost of reflow (check next session). That means that with more complex CSS rules the delay will increase.
If we set the
#sidediv
todisplay:none
that delay does not happen anymore but we cause a problem with the size of the "ace2_inner" as it's written here https://github.com/ether/etherpad-lite/blob/develop/src/static/css/iframe_editor.css#L125On both versions I'm running without any plugin.
Chrome dev-tools report for operation shown on the video
1.7.5
1.8.4
the rendering time increased a lot
look how long takes the "Layout" operation
Some interesting articles about the Theme
https://developers.google.com/web/fundamentals/performance/rendering/avoid-large-complex-layouts-and-layout-thrashing
https://gist.github.com/paulirish/5d52fb081b3570c81e3a
https://gist.github.com/paulirish/5d52fb081b3570c81e3a#more-on-forced-layout
The text was updated successfully, but these errors were encountered: