Misc sector loading optimizations

[eiriklegernaes]

Type of change

Description 📝

This PR contains optimizations found to reduce the lag spikes associated with sector loading:

• The main improvement comes from having CadManager consume sectors in batches. The slight delay in loading is not noticeable, but the reduction in lag spikes is. I suspect there's two main reasons why this is so effective, but it's hard to be sure:
  • Even when only a few sectors are unloaded, the call to gl.BufferSubData is likely to move a large portion of the buffer, since the sector may come from wildly different parts of the buffer. By batching the calls to consumeSector together, more of these CPU-side sector loading operations happen before rendering/updating the GPU buffer, leading to fewer overall calls/data transfer.
  • I suspect there's some javascript engine optimization that is able to kick in when the consumeSector function is called in a simple for-loop, but not when it's called repeatedly using a very tall rxjs call stack. The reason for this is that the CPU-part of sector loading appears much smaller in a performance profile in Chrome.
• By optimizing/specializing IndexSet.hasIntersectionWith for maps, and use this in setModelRenderLayers, some additional performance was gained.
• Even though it didn't lead to any measurable performance gain on my setup, I've kept a change that changes the usage for the buffers in MultiBufferBatchingManager from gl.STATIC_DRAW to gl.DYNAMIC_DRAW. Just seemed more inline with the actual usage.

Also, the batching of consumeSectors meant sectors would be loaded slightly after the progress reporting happened, which broke visual tests. This PR therefore contains a bit of plumbing to fix this.

How has this been tested? 🔍

In order to understand what actually made an impact, a benchmark was created by having the camera rotate around a medium sized asset in a deterministic fashion (0.6 rad/s, 30 sec total, continuous sector loading, 5 sec idle before start), while recording the frame-to-frame intervals. Although benchmarking like this is far from a science, it appeared to be fairly consistent as long as the environment (window mode, resolution, dev tools) were kept the same.

The spinner style updating was also found to cause major lag spikes (see benchmark below). This is not fixed in this PR, and should get it's own PR at some point. It is included when doing benchmarking, since disabling it gave much more consistent measurements.

The following breaks down the changes and their performance impact on my setup (Chrome, Ubuntu). All frame-to-frame measurements in ms:

Vanilla

	Avg	95th Percentile	99th Percentile	Max
Run 1	77.2	172.6	223.1	259.2
Run 2	81.4	183.6	211.4	244
Run 3	81.5	196	223	237.8
Run 4	87.8	215	242.6	266.6
Run 5	79.4	192.1	232.9	246

Vanilla + spinner disabled

	Avg	95th Percentile	99th Percentile	Max
Run 1	34.1	79.4	109.5	154.4
Run 2	33.4	81.2	102.4	122.6
Run 3	33.4	81.4	108.9	148.1
Run 4	35.3	83.5	104.4	128.3
Run 5	33.7	84.4	108.3	171

Vanilla + spinner disabled + sector batching

	Avg	95th Percentile	99th Percentile	Max
Run 1	19.7	44.8	83.5	117.5
Run 2	19.9	46.6	82.2	130.2
Run 3	19.9	44.3	88.1	128.8
Run 4	19.9	47.6	82.1	137.1
Run 5	20.0	47.7	91.2	138.5

Vanilla + spinner disabled + sector batching + IndexSet optimization

	Avg	95th Percentile	99th Percentile	Max
Run 1	18.9	43.9	61.5	75.6
Run 2	19.2	44.1	62.6	105.1
Run 3	19.4	45.9	67.4	113.9
Run 4	19.2	44.1	61.7	78.3
Run 5	19.2	44.6	62.9	80.9

[codecov]

Codecov Report

Merging #3674 (ac133e3) into master (f56fadb) will decrease coverage by 0.03%.
Report is 1 commits behind head on master.
The diff coverage is 76.11%.

@@            Coverage Diff             @@
##           master    #3674      +/-   ##
==========================================
- Coverage   71.73%   71.70%   -0.03%     
==========================================
  Files         346      346              
  Lines       33761    33798      +37     
  Branches     2652     2653       +1     
==========================================
+ Hits        24217    24236      +19     
- Misses       9437     9454      +17     
- Partials      107      108       +1     

Files	Coverage Δ
...ges/cad-model/src/utilities/GeometryBufferUtils.ts	98.88% <100.00%> (+0.09%)	⬆️
viewer/packages/utilities/src/indexset/IndexSet.ts	86.95% <100.00%> (-0.10%)	⬇️
.../cad-geometry-loaders/src/CadModelUpdateHandler.ts	83.13% <75.00%> (-0.48%)	⬇️
...ackages/rendering/src/utilities/renderUtilities.ts	90.20% <25.00%> (ø)
...er/packages/cad-geometry-loaders/src/CadManager.ts	64.28% <71.42%> (-0.03%)	⬇️
...es/cad-geometry-loaders/src/sector/SectorLoader.ts	92.26% <61.11%> (-5.59%)	⬇️

[christjt]

Amazing contribution! Give me a couple days to digest your findings.

[Strepto]

Interesting find on the Spinner, I guess an easy fix there would be to add

  set loading(loadingState: boolean) {
    if (this._loading == loadingState) return; <--- Dont do anything if the state is unchanged
    this._loading = loadingState;
    // snip

As I cant see anywhere else this should have a hot-path. Edit I see when orbiting something the spinner style changes from loading to nonloading very frequently, so I guess there is more to this.

[eiriklegernaes]

Yeah, the spinner loading state changes whenever RevealManager fires loadingStateChanged, which is surprisingly often. I did try your suggestion, but that still left too much performance on the table, so I left it as future work.

[christjt]

💯