Skip to content
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

layout trashing in WorkbenchLayout#layout #60031

Closed
jrieken opened this issue Oct 5, 2018 · 6 comments
Closed

layout trashing in WorkbenchLayout#layout #60031

jrieken opened this issue Oct 5, 2018 · 6 comments
Assignees
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Oct 5, 2018

  • record performance and reload
  • (expensive) layout trashing happens WorkbenchLayout#layout

screenshot 2018-10-05 at 17 10 21

I fiddled with this and it seems to be caused by these lines https:/Microsoft/vscode/blob/2f28e6ba8ae02a51d20e2c85fd8cd702fc8de37e/src/vs/workbench/browser/layout.ts#L547-L554

Question are

  • this looks like a workaround for something. is that still needed?
  • is there a better way to achieve the workaround, e.g. not having it during the main layout call?
@bpasero bpasero added this to the On Deck milestone Oct 5, 2018
@bpasero
Copy link
Member

bpasero commented Oct 5, 2018

I think this code is obsolete once #50853 is tackled, which I think @sbatten and @joaomoreno are going to look into.

@jrieken
Copy link
Member Author

jrieken commented Oct 5, 2018

rewriting the layout sounds like a bigger issue... isn't there a short term 'fix'? There is potential for saving lots of start-up perf time (~100ms)

@bpasero
Copy link
Member

bpasero commented Oct 5, 2018

@jrieken its not really a rewrite, rather using what the grid widget already provides and has battle-tested in the editor area.

@joaomoreno
Copy link
Member

Good investigation. But it would be a waste of time to go through that old code now... Let's close it since #50853 will be tackled pretty soon.

@jrieken jrieken reopened this Oct 9, 2018
@jrieken
Copy link
Member Author

jrieken commented Oct 9, 2018

I know everyone is busy with other things but upon a second look I don't this is related to layouting...

Those lines (some of them are from 2015) try to recover from some chrome behaviour we seem to have observed but not understood. It's not really part of layouting but I guess the layout-function was a convenient place for that layout-fixing-when-something-weird-happened-logic. With that in mind I think changing to a grid or whatever logic won't help.

I have removed those lines and it seems nothing has changed, I could also not reproduce #6810 without those lines. So, I call it good without those lines and I leave it up to @bpasero to double check.

@bpasero
Copy link
Member

bpasero commented Oct 10, 2018

If this still happens, an alternative solution would be to move this code into the next animation frame and thus take it out of the critical startup path.

jrieken added a commit that referenced this issue Oct 18, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants