-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
Editor layout is called 5 times opening 1 file #112630
Comments
This is bad for the work to support vertical tabs in #106448 because the |
I remember spending a lot of time making sure this wouldn't happen, as far as the grid goes. Sucks if it broke. I can take a peek. |
I actually see 6 calls to layout, this is preposterous. The last 2 are calls via |
Currently investigating the first 4 But those last 2 |
@joaomoreno I agree with reducing calls to I am not sure about the |
@bpasero This is the stack of that call:
Which runs after all other 5 calls run, so it seems it's just unnecessary, given the grid within has already been laid out correctly. |
Repro case:
🐛 6 calls to There are 3 separate causes to the 6
|
Thanks, I will look into the |
@joaomoreno Do you mean when breadcrumbs are enabled/disabled or also when their content changes? In the case of the former we might assume that breadcrumbs are enabled. Surely this code was written when they were off by default. Out of curiosity: it is always good to prevent unneeded calls, but do have a chance to protect yourself from such calls, e.g early return when layout is called with the input? I feel without that you are repeatedly encountering this because stuff just rots |
Yeah that makes sense. What I observed was that the breadcrumbs code was assuming something has changed, even though nothing changes regarding breadcrumbs when going from 0 to 1 editor.
Totally. This occurs now due to rot. Yeah I'll see if I can protect a bit better here. |
Breadcrumbs does that because per editor input it might be hidden, e.g when you open an extension editor it needs to hide, and that needs to relayout to return the screen real-estate (or take it back). Tho, surely can be done smarter |
@jrieken I think But in reality you return
|
@jrieken thanks, I pushed another change to avoid repeated relayout calls from breadcrumbs, specifically this call is no longer needed...
...because ever since tabs can wrap, there is logic in the layout call that will trigger a
And that should take care of breadcrumbs too. I tested this briefly (jumping between normal text editor and untitled file) and couldn't find any issues, but maybe you could also give it a test. The remaining layout calls seem to originate from the grid, so leaving up to Joao to decide wether we want / can improve that. |
Cool. I will keep an eye open |
"fixed", at least breadcrumbs isn't triggering this eagerly anymore. Tho, when opening 1 file I count 6(!) layout calls and I'd say opening files is a more common scenario. @joaomoreno question: layout is about sizing and positioning child elements, right? So, by no means I support calling layout unnecessarily but isn't layout a good candidate for caching dimensions and then ignoring subsequent calls with the same dimensions? |
Definitely, the question is where should that be? In the widgets themselves? In the widget components? In the workbench components? Everywhere? 🙈 |
So weird. I kept seeing only one. I commented all my settings, then I started seeing 2. I reverted my settings change and now I see 2 all the time. Second one is the relayout mentioned above:
|
We figured out in Slack:
|
But my initial steps indicate the scenario, it is not startup, but opening a file:
I see the following layout calls now from a fresh workspace when I open the first file:
That is 5 |
Right, looking again. |
The last one is interesting for maybe me+Jo, we first assume a height of |
We should just assume that breadcrumbs are on - as they are by default- or read the config sooner. What I don't know is what we are laying out here. Isn't the title, breadcrumbs, and the editor-widget inside a single grid element and isn't that element already sized? What are we resizing/re-layouting here? |
As for the thing I am looking at (the "relayout" triggered from the title control), that one is only triggering a relayout of the widgets inside the grid, not the entire grid. And the reason is this:
|
Why is this? Entire complex widgets (splitview and list) layout in a sync fashion and a title bar doesn't? Can't you actually compute how much height you need instantly? |
@joaomoreno I am all up for changing the tabs widget to use a list, but there are some things that it does which made me put the layout code into a "schedule at next animation frame"
|
It's a bit unfair that the async layout is also used for breadcrumbs because that is exactly why the relayout triggers: we only figure out that breadcrumbs are there when the animation frame executes, but we could run that part of the layout sync. I think a better way would be to only keep that code async where we know that DOM properties are accessed that can trigger a browser layout. |
Some FYI - running at next animation frame doesn't give you a guarantee that the DOM is pretty and that asking for layout info isn't expensive. It might well be that someone already modified the DOM so that an expensive re-compute is required but it is much less likely. I found this nice article a while back (https://developers.google.com/web/fundamentals/performance/rendering/avoid-large-complex-layouts-and-layout-thrashing) I then added |
@jrieken interesting, are you suggesting that I should be then using |
Yeah, the idea is to do all measuring on a "fresh" layout and do all modifications after that. Tho, we need to onboard all components to see the full effect of this |
Fixed this all the way down to two layout calls: the actual one and the delayed one from @bpasero. |
👏 ❤️ |
This looks a lot lot better now, I verified that clicking on a file when no file is opened now only calls the However, I went down the path trying to avoid the But the good news is, that this call of
Since the |
Steps to Reproduce:
console.log
intoeditorGroupView.layout
=>
layout
is called 4 timesFirst one seems legit to me:
But not sure about these:
Also adding Isi since this is maybe the centered grid layout.
The text was updated successfully, but these errors were encountered: