GridLayout perf improvements by sbatten · Pull Request #79614 · microsoft/vscode

sbatten · 2019-08-22T04:21:24Z

Summary

Update SplitView to allow reconstruction given a set of children views and descriptors.
Update GridView to allow reconstruction given a set of node descriptors.
Update SerializableGrid to use new reconstruction api (use deserialize2 to avoid mucking with anything the editor grid is doing)
Updated the testing index.html page to allow playing with it.
Adopt this in layout.ts

I've played around with this and it feels good and shows a great perf improvement, but as we all know testing ones own code can suffer from tunnel vision.

I know @joaomoreno is out so I'm looking for some help getting this tested and safely in.

Perf Improvments

The primary goal of this work is to improve workbench startup time. As measured on my machine, the current grid layout performs ~40ms slower than the legacy layout on initial startup time. I am using the startup performance command to look at markers. I have the below patch for more fine grained measurement. On my machine, these changes recover >75% of that lost time.

Why was it slow?

The way we deserialize today is by adding views individually at the grid level (not grid view, or splitview level). What that means is that every addition of a view triggers a layout call for that view and additionally for the rest of the views. Joao did some work to make this faster by trying to delay layout calls until the end of the deserialization, but the issue remained because not enough information was initialized during the deserialization process so the initial layout call triggered many recalculations. Instead of dealing with layout controllers, the proper fix as Joao and I discussed was to bring this logic down to the individual view types. Now the initial layout calls layout on each part exactly once during startup.

diff --git a/src/vs/workbench/browser/workbench.ts b/src/vs/workbench/browser/workbench.ts
index 2506a50c8c..2098b5de11 100644
--- a/src/vs/workbench/browser/workbench.ts
+++ b/src/vs/workbench/browser/workbench.ts
@@ -156,11 +156,13 @@ export class Workbench extends Layout {
 				// Render Workbench
 				this.renderWorkbench(instantiationService, accessor.get(INotificationService) as NotificationService, storageService, configurationService);
 
+				mark('willLayoutWorkbench');
 				// Workbench Layout
 				this.createWorkbenchLayout(instantiationService);
 
 				// Layout
 				this.layout();
+				mark('didLayoutWorkbench');
 
 				// Restore
 				try {
diff --git a/src/vs/workbench/contrib/performance/electron-browser/perfviewEditor.ts b/src/vs/workbench/contrib/performance/electron-browser/perfviewEditor.ts
index b0a1861b2c..795a0625a1 100644
--- a/src/vs/workbench/contrib/performance/electron-browser/perfviewEditor.ts
+++ b/src/vs/workbench/contrib/performance/electron-browser/perfviewEditor.ts
@@ -161,6 +161,7 @@ class PerfModelContentProvider implements ITextModelContentProvider {
 		table.push(['restore panel', metrics.timers.ellapsedPanelRestore, '[renderer]', metrics.panelId]);
 		table.push(['restore editors', metrics.timers.ellapsedEditorRestore, '[renderer]', `${metrics.editorIds.length}: ${metrics.editorIds.join(', ')}`]);
 		table.push(['overall workbench load', metrics.timers.ellapsedWorkbench, '[renderer]', undefined]);
+		table.push(['workbench layout', metrics.timers.ellapsedLayout, '[renderer]', undefined]);
 		table.push(['workbench ready', metrics.ellapsed, '[main->renderer]', undefined]);
 		table.push(['extensions registered', metrics.timers.ellapsedExtensionsReady, '[renderer]', undefined]);
 
diff --git a/src/vs/workbench/services/timer/electron-browser/timerService.ts b/src/vs/workbench/services/timer/electron-browser/timerService.ts
index 139822af63..8af849f389 100644
--- a/src/vs/workbench/services/timer/electron-browser/timerService.ts
+++ b/src/vs/workbench/services/timer/electron-browser/timerService.ts
@@ -158,6 +158,8 @@ export interface IStartupMetrics {
 		 */
 		readonly ellapsedAppReady?: number;
 
+		readonly ellapsedLayout?: number;
+
 		/**
 		 * The time it took to generate NLS data.
 		 *
@@ -399,6 +401,7 @@ class TimerService implements ITimerService {
 				ellapsedViewletRestore: perf.getDuration('willRestoreViewlet', 'didRestoreViewlet'),
 				ellapsedPanelRestore: perf.getDuration('willRestorePanel', 'didRestorePanel'),
 				ellapsedWorkbench: perf.getDuration('willStartWorkbench', 'didStartWorkbench'),
+				ellapsedLayout: perf.getDuration('willLayoutWorkbench', 'didLayoutWorkbench'),
 				ellapsedExtensionsReady: perf.getDuration(startMark, 'didLoadExtensions'),
 				ellapsedTimersToTimersComputed: Date.now() - now,
 			},

…itview

isidorn · 2019-08-22T13:57:28Z

@sbatten hi! This looks like nice work and I will gladly review this and test it with you.
This should be platform independent but I will try it out on os x.

Do you have a scenario where the previous solution was showing this slow perf? How can we easily measure that this new approach is an improvement?
Apart from that I plan to do a thourgh testing of all the layouting scenarios. Let me know if you would like me to focus on something specific.

isidorn · 2019-08-22T14:05:10Z

src/vs/base/browser/ui/grid/gridview.ts


-	addChild(node: Node, size: number | Sizing, index: number): void {
+	addChild(node: Node, size: number | Sizing, index: number, skipLayout?: boolean): void {
 		if (index < 0 || index > this.children.length) {


skipLayout is not being used. Why are we adding it?

isidorn · 2019-08-22T14:05:37Z

test/splitview/public/index.html

@@ -31,51 +31,131 @@
 	<script>


Great that you added tests! I see they pass, I will not review them.

@isidorn Everything in test/splitview are just visual manual tests, like a playground, you can safely ignore that. Proper unit tests are in place to make sure all this works.

isidorn · 2019-08-22T14:06:58Z

src/vs/base/browser/ui/splitview/splitview.ts

+				const snappedBefore = typeof snapBeforeIndex === 'number' && !this.viewItems[snapBeforeIndex].visible;
+				const snappedAfter = typeof snapAfterIndex === 'number' && !this.viewItems[snapAfterIndex].visible;
+
+				if (snappedBefore && collapsesUp[index]) {


I do not understand the collapsesUp and collapsesDown change.
This does not seem related to the serialization in the grid or am I missing something?

this change fixes a bug that I thought was only in my branch but it actually reproduces in insiders as well, I can pull this out, but you can repro the issue by hiding the sidebar, toggling its position, then trying to drag it out (unsnap it). It will not unsnap. If needed, I can move this to a separate bug fix.

Yeah I'd just put this one in master directly.

isidorn · 2019-08-22T14:09:26Z

src/vs/base/browser/ui/splitview/splitview.ts

 	setVisible?(visible: boolean): void;
 }

+export interface ISerializableView extends IView {


This is introducing the concept of serialization to the SplitView.
I think the SplitView should have no notion of this concept, but instead the clients of the SplitView

@isidorn This is what @sbatten and I discussed: the splitview needs to be serializable to be able to hydrate in a performant manner.

isidorn · 2019-08-22T14:11:21Z

src/vs/base/browser/ui/splitview/splitview.ts

 		this.style(options.styles || defaultStyles);
+
+		// We have an existing set of view, add them now
+		if (options.descriptor) {


Why is the splitView doing this? Why doesn't the client simply construct the new SplitView and add all the deserialized views and says skip the layout.
If we went with this approach I think we can dump the ISplitViewDescriptor

SplitView was kind of an easy case for adding skipLayout, but I don't actually like having that as part of its public api, especially since it doesn't match well with GridView's branch node api. To make this look more correct optically and avoid people calling with skiplayout, I've made that a private call.

We don't want to add view by view and skipping the layout until the end. This will still cause all the math to happen in the background. Plus it will expose a terrible skipLayout API. As per comment above, we want a serializable Splitview which can deserialize in a single layout call.

isidorn · 2019-08-22T14:17:33Z

src/vs/base/browser/ui/grid/gridview.ts

 		return this._getViews(node, this.orientation, { top: 0, left: 0, width: this.width, height: this.height });
 	}

+	static deserialize<T extends ISerializableView>(json: ISerializedGridView, deserializer: IViewDeserializer<T>, options: IGridViewOptions = {}): GridView {


I feel like this does not belong here.
So this is a GridView which so far had no notion of serialization and we are adding it.
I thought that is the point of the SerializedGird.
Same as my previous comment, serialization should not be done on this level but on one level higher it feels.

With this PR both the Splitview and Gridview will support serialization. The idea is to also get rid of the SerializableGrid, since the Grid will be able to deserialize.

isidorn · 2019-08-22T14:23:14Z

src/vs/base/browser/ui/grid/grid.ts

 		super();
-		this.gridview = new GridView(options);
+
+		if (view instanceof GridView) {


Can you please clarify this new case when the view is instanceof GridView. Since this seems to not be covered before

Grid historically created a grid view and then added views, we know this has pitfalls due to layouts for each added view. This allows us to provide a preconstructed one.

Ya this is weird, I'd rather have a public deserialize method for this. But that's just API preference.

isidorn · 2019-08-22T14:24:37Z

@sbatten I did an initial review and added comments - which are more like questions. So it would be great if you could adress them or explain to me more about the reasoning behind why we choose to do it in this way - if it is easier we can also talk via Teams (I should be online for another 90 minutes).

Can you also explain more about why the new serialisation tehnique is better than the previous one - will make it easier for me to digest the code since I have no prior deep understanding.
Thanks a lot!

sbatten · 2019-08-22T16:06:46Z

@isidorn I've updated the PR description with more context and added some responses to your comments as well. Lmk if more information is desired.

joaomoreno · 2019-08-23T13:18:14Z

Can you also explain more about why the new serialisation tehnique is better than the previous one - will make it easier for me to digest the code since I have no prior deep understanding.

@isidorn Deserializing a splitview/grid today requires additively modifying the widget from an empty state into the fully reconstructed state, adding view by view. This is expensive because in runs through all the layout code in O(n2). Even if we skip calling layout to the views, it will still do all the math. The changes allow to deserialize both a splitview and a grid in O(n) by doing it all at once.

joaomoreno

@sbatten I'm not a big fan of deserialize2. Since this is a PR, just go ahead and replace the current deserialize code with the new approach. This will help you validate the implementation in two ways: by benefiting from the already existing test suite and by seeing it work in the editor grid. You're going to have to do that work eventually, better do it today than in debt week.

joaomoreno · 2019-08-23T13:20:37Z

src/vs/base/browser/ui/splitview/splitview.ts

+				const snappedBefore = typeof snapBeforeIndex === 'number' && !this.viewItems[snapBeforeIndex].visible;
+				const snappedAfter = typeof snapAfterIndex === 'number' && !this.viewItems[snapAfterIndex].visible;
+
+				if (snappedBefore && collapsesUp[index]) {


Yeah I'd just put this one in master directly.

joaomoreno · 2019-08-23T13:21:20Z

src/vs/base/browser/ui/splitview/splitview.ts

+		this.state = State.Idle;
+
+		if (typeof size !== 'number' && size.type === 'distribute') {
+			this.distributeViewSizes();


distributeViewSizes is expensive, we very likely also want to skip this if skipLayout.

joaomoreno · 2019-08-23T13:24:41Z

src/vs/base/browser/ui/splitview/splitview.ts

+	views: {
+		visible?: boolean;
+		size: number;
+		view: IView;


@sbatten I might be wrong but I believe this won't work in the big picture. Remember that a gridview is a tree of splitviews. In order to fully deserialize a whole gridview in one step, you're very likely going to have to use the deserializer pattern in which a fromJSON method is called, even down in the splitview.

the grid view deserializes views using a depth first search so this shouldn't be an issue

isidorn · 2019-08-23T13:45:20Z

@joaomoreno thanks for the review
@sbatten since Joao did they review I suggest you tackle his comments and simply ignore mine.
I would be happy to help with intensive testing on Monday. So just let me know when you would like me to test this to best help. Thanks

sbatten · 2019-08-24T02:37:44Z

@isidorn I've addressed @joaomoreno's feedback and now have a single deserialize call. With this change and some modification, the new deserialize API passes all unit tests and sanity manual tests. I am more confident in it now and think it should be ready for testing.

@joaomoreno Interesting point about the latest commit. One of the unit tests was failing because it was testing that the grid would resize all views proportionally on the first layout call. This logic is actually counter to how the workbench should work. We don't usually want to scale out the sidebar or panel when we get some extra space. And today if the window is smaller than the panel, it will just take most of the screen in legacy layout.

With that observation, I have reserved this proportion-preserving behavior only for grids with proportonalLayout enabled. That means I don't need to do anything special in the initial layout, because it is functionally no different from any other layout. I simply needed to initialize the proportions in SplitView to make this work.

isidorn · 2019-08-26T07:34:23Z

@sbatten thanks for adressing the feedback. I plan to test the branch out today. Will update this comment with my findings.

isidorn · 2019-08-26T10:33:33Z

@sbatten I have tested this and it works nicely. I am especially happy about the Panel maximize now being able to take the full view.
However I still suggest that after merging this in that you create a test plan item with a large complexity and you assign it to me. So that I get more time tomorrow to test this.

As for the performance, when running out of source from this branch with your new serialization improvements (after 4 runs on average) I get 2244 ms for the Workbench Load.
When disabling the grid layout via a flag, I get 2205 ms. Thus we have a startup slowdown of 2%. I tested this with two editor groups side by side the sidebar and panel open.
This slowdown seems ok for me. However it would be great if we could further improve this before we push this out to stable to not get a bump in performance numbers.
edited: my original numbers were off (due to laptop weirndess, on second and third measure I got the 2% slowdown)

Since the layout seems to works and we should start self hosting on this I am approving this PR.

Thanks for the nice work

isidorn · 2019-08-26T16:28:03Z

fyi @jrieken for potential minor startup perf hit. Based on my local measuring workbenchLoad is 2% slower with the new grid layout.

sbatten · 2019-08-26T16:59:03Z

But this should still be a perf gain when comparing to today's insiders

* add deserialization to splitview * add new grid deserialization test to index.html * working mostly * branchnode should invert orthogonality in splitview * fix issue with sidebar snapping * cleanup and make splitview api more explicit * hook up children sash reset * adopt single deserialize (PR feedback)

sbatten added 7 commits August 16, 2019 16:54

add deserialization to splitview

a77c65a

add new grid deserialization test to index.html

325a6fb

Merge branch 'master' into sbatten/perf/splitview

1c4c36c

Merge remote-tracking branch 'microsoft/master' into sbatten/perf/spl…

6baf42c

…itview

working mostly

006ef5e

branchnode should invert orthogonality in splitview

0d85761

fix issue with sidebar snapping

1628eb3

sbatten requested review from bpasero and joaomoreno and removed request for joaomoreno August 22, 2019 04:21

sbatten self-assigned this Aug 22, 2019

sbatten added this to the August 2019 milestone Aug 22, 2019

isidorn self-requested a review August 22, 2019 09:13

isidorn requested changes Aug 22, 2019

View reviewed changes

isidorn reviewed Aug 22, 2019

View reviewed changes

cleanup and make splitview api more explicit

905c0f9

joaomoreno requested changes Aug 23, 2019

View reviewed changes

sbatten added 3 commits August 23, 2019 17:18

Merge branch 'master' into sbatten/perf/splitview

15546b1

hook up children sash reset

0647150

adopt single deserialize (PR feedback)

49b6838

isidorn approved these changes Aug 26, 2019

View reviewed changes

Merge branch 'master' into sbatten/perf/splitview

e6752f9

sbatten merged commit 61c52d0 into master Aug 26, 2019

sbatten deleted the sbatten/perf/splitview branch August 26, 2019 15:12

sbatten mentioned this pull request Aug 26, 2019

Test Grid Layout #79845

Closed

3 tasks

anthonydresser mentioned this pull request Sep 13, 2019

Merge from vscode 64980ea1f3f532c82bb6c28d27bba9ef2c5b4463 microsoft/azuredatastudio#7206

Merged

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Conversation

sbatten commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Perf Improvments

Why was it slow?

Uh oh!

isidorn commented Aug 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isidorn commented Aug 22, 2019

Uh oh!

sbatten commented Aug 22, 2019

Uh oh!

joaomoreno commented Aug 23, 2019

Uh oh!

joaomoreno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isidorn commented Aug 23, 2019

Uh oh!

sbatten commented Aug 24, 2019

Uh oh!

isidorn commented Aug 26, 2019

Uh oh!

isidorn commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isidorn commented Aug 26, 2019

Uh oh!

sbatten commented Aug 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbatten commented Aug 22, 2019 •

edited

Loading

isidorn commented Aug 26, 2019 •

edited

Loading