Batch render DOM elements to avoid reflow #10530

zarov · 2021-04-01T07:52:19Z

I noticed there was a lot of DOM reflow in case someone adds markers, popups and other DOM-based elements to the map. This can be visible in the debug/markers.html page. You can read how to avoid this problem here.

What I am proposing is adding a DOM rendering queue on the map, similar to the current render queue, in which you can add all DOM-related layout changes. Note that I tried to use the existing render queue, but the result was that DOM elements were updated too late, so there were moving with a delay on screen.

I am not really sure if this change fits the current architecture, so feel free to direct me where to move this new behavior.

There may also be others places where to apply those changes, as I applied it only where I thought it was relevant.

Performance

You can see below the difference in the call stack in the markers.html page. From manual testing, I would say that the concerned part has seen its time reduced by 5x-10x.

Before

After

Launch Checklist

briefly describe the changes in this PR
include before/after visuals
manually test the debug page

CLAassistant · 2021-04-01T07:52:23Z

All committers have signed the CLA.

mourner

This is a great improvement, thanks so much for the contribution! Added a few nits inline.

mourner · 2021-04-02T12:16:40Z

src/ui/popup.js

-        DOM.setTransform(this._container, `${anchorTranslate[anchor]} translate(${offsetedPos.x}px,${offsetedPos.y}px)`);
-        applyAnchorClass(this._container, anchor, 'popup');
+        const container = this._container;
+        const _anchor = anchor;


Nit: given that the closure captures the anchor variable and it's not modified later in the method, and that the arrow function retains the context of this, why do we need to define aliases here?

Oops, I edited the comment here but the old version got posted somehow — so, now I see that the DOM elements are aliased so that it doesn't break in case a task is run after the object is removed from the map, but it's not obvious, so might need additional comments to avoid maintainers tripping up on this. Alternatively, we could look into whether this can be refactored to avoid the issue differently — e.g. perhaps marking a task as cancelled when a corresponding object is removed, or maybe adding some guards for the elements inside the closure.

To be honest, I got an error with flow regarding anchor about it being null in the arrow function and I didn't understand how that can be possible, hence the alias.

It would be better however to see how it can be refactored. What about storing the task ID in the popup, as there should always be only one? Considering the total number of popup, it won't be a burden and then we could cancel the task in remove(). Otherwise another simple solution that seems to work is by simply adding if (this._container && anchor) in the arrow function.

mourner · 2021-04-02T12:18:12Z

src/ui/control/scale_control.js

    }
 }

-function setScale(container, maxWidth, maxDistance, unit) {
+function setScale(map, container, maxWidth, maxDistance, unit) {


Nit: it's better to pass the smallest scope necessary for the function — in this case, queue instead of the whole map object. Makes refactoring easier later.

Thanks I totally missed this!

mourner · 2021-04-05T08:18:57Z

src/ui/control/scale_control.js

    } else {
-        setScale(container, maxWidth, maxMeters, map._getUIString('ScaleControl.Meters'));
+        setScale(container, maxWidth, maxMeters, map._getUIString('ScaleControl.Meters'), map._domRenderTaskQueue);


Nit: would be nice to extract this to const domQueue = map.domRenderTaskQueue to make it a little less verbose.

mourner · 2021-04-05T08:19:31Z

src/ui/marker.js

-        DOM.setTransform(this._element, `${anchorTranslate[this._anchor]} translate(${this._pos.x}px, ${this._pos.y}px) ${pitch} ${rotation}`);
+        const el = this._element;
+        const pos = this._pos;
+        const anchor = this._anchor;


You can get rid of aliases here too (and check for just this._element).

This is embarrassing, I really thought I removed it!

mourner · 2021-04-05T08:19:56Z

src/ui/popup.js

-        DOM.setTransform(this._container, `${anchorTranslate[anchor]} translate(${offsetedPos.x}px,${offsetedPos.y}px)`);
-        applyAnchorClass(this._container, anchor, 'popup');
+        this._map._domRenderTaskQueue.add(() => {
+            if (this._container && anchor) {


Nit: anchor is always truthy here so no need to check.

I agree, but flow is not understanding this. Is there any flag I can maybe add to make it understand?

Same goes in marker.js btw.

See this previous job on CircleCI for reference

ghost · 2021-04-07T13:21:58Z

@zarov can you also license your change under 3-BSD or submit it to https://github.com/maplibre/maplibre-gl-js ?
Otherwise such FOSS forks can't legally use your changes.

* Add fog stylespec * Add fog type specification * Add fog stylespec validation Lint * Add setFog() and getFog() APIs * Add debug page * Add note to validate fog * Add Fog preludes and inject preprocessor defines when enabled Fix Lint Fix Flow drop: Test Fog define * Inject shader preludes and connect to globally defined fog uniforms Fixup * Apply fog to sky layers Remove unnecessary shader statement * Apply fog to terrain render * Fix fog on sky gradient layers * Apply fog to fill-extrusion layer * Apply fog to fill layer * Apply fog to fill-extrusion-pattern layer Fixup shader statement * Apply fog to fill-outline, fill-outline-pattern, fill-pattern layers * Apply fog to background layer * rename posMatrix to projMatrix * Add cameraMatrix * add separate cameraMatrix * Calculate worldsize for cameraMatrix based on camera distance distance to sealevel Fix flow and lint errors * Simplify shader statement, remove extra division by w u_cam_matrix is not projective and w is always 1 for points * Add fading in of fog around 60 deg pitch Fix lint errors * Improve fog demo example and add GUI controls * Fog tile culling in tile cover Fixup comment * Remove extraneous mult Remove unused * Simplify fog tile culling Fix lint and flow * Apply correct fog+blending in shaders (#10496) * Make an attempt to implement consistent fog logic * Add comments * Apply premultiplied fix across shaders * Clean up gamma correction * Simply GLSL defines and fix shader compile error * Fix syntax error and remove unused gamma correction * Add fog to circles * Apply fog to background pattern and hillshade * Clean up a small shader optimization * Add dithering to fog (#10506) * Add basic dithering to fog * Add temporal offset for fog * Make uniform naming consistent * Code golf the shaders just a little * Separate dithering from fog function * Rework the fog falloff function (#10515) * Rework the fog falloff function * Rework the fog function * Add some spec validation * Support fog on markers (#10516) * Apply fog to Markers * Fix lint flow and cleanup * Fix opacity not updating for terrain and fog after updating the spec properties * Add map.getFogOpacity API * Code style * Evaluate opacity less often * Update per rebased changes * Minor tweak * Update src/style/fog.js Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Update src/ui/marker.js Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Update src/ui/map.js Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Update variable name per review * Fixup invalid commit from github * Address review comments * Update per review Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> Fix popup tip not being occluded * Fog-sky blending (#10527) * Improve fog blending * Clean up blending * Remove fog-sky blending fix Fix tests and low * Do not cull tiles crossing over the horizon during fog culling phase This prevents sudden pop in and out for tiles within the fog culling range * Use fog color as default context clear color when enabled This prevents unaffected fragments resulting in empty holes in location where we do not draw fragments * Cull at 80% of the distance between fog start end fog end This leaves a non-noticeable 2% fog opacity change threshold and reduces the distance before we start culling * Add style diffing support and unit tests fog setFog and getFog * Cutoff fog-opacity from the style specification * Fix a bug using stale tile cover result due missing update to fog ranges This issue was visible when updating fog properties and an incorrect tile cover run would potentially leave invalid tiles on the viewport * Switch fog to use window-scaled units (#10533) * Switch fog to use window-scaled units * Update style spec for fog range * Update fog validation tests to match new limits * Update fog validations * Add haziness property to fog (#10537) * Add haziness to fog * Fix flow types * Apply PR feedback * Update marker opacity to use viewport-scaled height (#10542) * Update marker opacity to use viewport-scaled height * add units to variable names to make intent clear * Code cleanup: Simplify getDistanceToSeaLevel + remove extraneous transform variable * Add unit tests for furthestTileCorner * Add unit tests for coveringTiles with fog culling * Add basic symbol clipping when fog opacity is high (#10541) * Rename cameraMatrix --> fogMatrix (#10544) * Offload terrain fog to vertex shader (#10549) * Offload terrain fog to vertex shader * Don't set haze uniforms unless haze is present * Remove haze varying unless haze is used * Disable fog code path on the 2d case * Remove redundant uniforms * Simplify fog render logic Co-authored-by: Karim Naaji <karim.naaji@gmail.com> * Add documentation of the public facing fog APIs + revise defaults (#10545) * Add documentation of the public facing APIs * Fixup missing `}` * Update fog range docs * Disable haze by default and fix mixed up haze properties * Update src/style-spec/reference/v8.json Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Update src/style-spec/reference/v8.json Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Update src/style-spec/reference/v8.json Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Update src/style-spec/reference/v8.json Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Update src/ui/map.js Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Extract getAABBPointSquareDist from fog culling and add unit tests * Use implicit vec3.transformMat4 division by w instead of explicit one * Add fog as part of the release testing * Add fog culling render test * Add sky-blend render tests * Add more style diffing unit tests and coverage around setFog/getFog public APIs * Add unit tests for getFogOpacity * Add basic terrain render tests * Add fog marker occlusion unit tests Remove extraneous note * Add 2D tests for fog (#10566) * Add 2D render tests for fog * Test fog shaders * Swap raster test * Add half float heatmap render test image * Increase test allowance for raster test * Add more fog style spec render test coverage * Fix shader preprocessor define leading to invalid shaders on production build variants (#10582) * Prevent shader stripping issue on production builds Always use same function signature independently of shader variant * Use const for the unused param * Directly use inlined parameter * Fixup * Fix render tests Cannot use const as 'out' or 'inout' parameters. * Fog style spec changes (#10590) * Clean up fog style properties * Present a more reasonable example * Fix fog tests and validations * Improve haze wording * Apply review feedback: getFogOpacity -> queryFogOpacity * Apply review feedback: Keep seperation of DOM-based and non-DOM based map element updates Defer marker opacity update in DOM task queue * Fix fog sky blending (#10578) * Clean up fog sky blending * Add fog sky blending fix to cpu side Fix post rebase flow * Better consistency in naming conventions * Apply review feedback: Remove extraneous function call * Apply review feedback: Rename sky-blend -> horizon-blend * Apply review feedback: Increase range of pitch fading to be less aggressive * Simplify haze evaluation (#10606) * Simplify fog evaluation * Switch to approximate gamma conversion * Factor out haze application * Document the haze_apply function * Remove backticks * Update src/shaders/_prelude_fog.fragment.glsl * Apply review feedback: Consolidate shader code that can be shared in shared prelude * Remove haze (#10613) * Remove haze * Remove density * Catch a couple more small cleanup items * Fix broken shader * Tighten/restore fog culling * Update src/render/painter.js * Revert fog opacity regression from #10578 with CPU side evaluation of horizon blend * Demo: Update default location * Revise comments and cleanup shader code for readability - Also removes extraneous branching with min in fog_apply_premultiplied * Share fog varying globally in new shader prelude * Revise comments and naming * Code cleanup for better readability * Fix validation error on complex style expression of fog color validation * Fix validation error on complex style expression of fog range validation and add validation tests * Simplify fog validation checks for expression(s) * Switch fog from percentage window height to CSS vh units (#10632) * Switch fog from percentage window height to CSS vh units * Fix tests * Fix more tests * Fix still more tests * Update src/style-spec/reference/v8.json * Don't leave alpha of fog color unused (#10633) * Don't leave alpha of fog color unused * Apply review comments: Simplify flow, reduce uniforms * Fix naming * Update docs * Fix flow * Update all render tests to use new units * Fixup consistent naming * Add symbol clipping render test * Sample terrain to estimate de facto camera scale (#10621) * Continuously adjust the fog scale by raycasting * Remove unused function * Fix linter errors * Name thresholds for clarity * Hard-code sampling pattern * Rearrange conditional * Adjust sampling by horizon line * Replace arbitrary constant with epsilon * Remove stray import * Simplify logic * Only calc fog matrices on avg elevation change * Fix cloning of transform * Fix bug in state management * Fix linter errors * Remove debug code * Move samples points inside function * Fix accidental call to calcMatrices * We still love you, flow * Clarify code * Add tests for EasedVariable * Add one more test * Fix default per latest changes and add render tests * Fix alpha composing of horizon blend with sky gradient (#10640) Make sure to un/post multiply alpha before/after applying sky gradient contribution, as color ramps are premultiplied-alpha colors. This prevents washed out colors when using transparency. * Account for FOV in fog (#10629) * Account for FOV in fog * Fix a typo * Fix flow errors * Fix a factor of two to make tests pass * Fix bad merge * Fix tests * Shift fog range zero to the map center * Fix debug * Fix ranges to account for fov * Remove unused variable * Apply review feedback: Directly update marker style opacity * Update per latest discussion: Use nameless units to prevent confusion when trying to reason about them. As the units of the range are representing 3d distances on a sphere centered around camera center point, they can hardly be reasoned about in terms of window height. * Fixup naming consistency * Fix race condition in fog terrain sampling (#10642) * Fix race condition in fog terrain sampling * Use frame start time stamp * Update render test expectation * Remove fog vh units (#10648) * Remove fog vh units * Fix floating point errors in test expectations * Fix geo test * Add fog demo and transition fixes (#10649) * Skip unnecessary uniform code path when terrain is rendering to texture On a typical frame with terrain this was called ~200 times, where only 20 were necessary (~10%) * Fix invalid transition evaluation When `delay` was undefined but not `now`, properties.begin and properties.end were assigned incorrect values, leading to an immediate transition interpolation. This was noticed when testing transitions on fog. * Fix style.hasTransition() when fog is transitioning values * Add a fog demo for release * Directly use DOM queue rencently introduced by #10530 * Apply review feedback: Directly clear the timer here instead of unsetting it * Apply review feedback: Fix nits, mark queryFogOpacity private until we need to expose it publicly * Update src/util/util.js Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Update src/util/util.js Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> * Apply review feedback: Fix nits * Apply review feedback: Simplify matrix initialization * make private methods private for docs Co-authored-by: Arindam Bose <arindam.bose@mapbox.com> Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com> Co-authored-by: Ansis Brammanis <ansis@mapbox.com>

Batch render DOM elements to avoid reflow

846520d

Fixed flow and unit testing for DOM batch rendering

bbeb2f1

mourner self-assigned this Apr 1, 2021

mourner requested changes Apr 2, 2021

View reviewed changes

mourner requested changes Apr 5, 2021

View reviewed changes

Removed aliases after review

27ab023

zarov force-pushed the layout-thrashing branch from dc99ebf to 27ab023 Compare April 5, 2021 13:02

mourner approved these changes Apr 7, 2021

View reviewed changes

mourner merged commit 925f9a3 into mapbox:main Apr 7, 2021

ghost mentioned this pull request Apr 7, 2021

How to use new contributions under the 3-Clause BSD license, without asking for permission on each PR #10543

Closed

zarov mentioned this pull request Apr 13, 2021

Bypass the DOM rendering queue if map is idle #10567

Merged

karimnaaji mentioned this pull request Apr 19, 2021

Add a configurable Fog effect #10564

Merged

9 tasks

karimnaaji added this to the v2.3 milestone May 6, 2021

karimnaaji added a commit that referenced this pull request May 6, 2021

Directly use DOM queue rencently introduced by #10530

158dece

adamkovalsky mentioned this pull request Sep 15, 2021

Markers show in the top-left corner until map is loaded #11020

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch render DOM elements to avoid reflow #10530

Batch render DOM elements to avoid reflow #10530

zarov commented Apr 1, 2021 •

edited

Loading

CLAassistant commented Apr 1, 2021 •

edited

Loading

mourner left a comment

mourner Apr 2, 2021

mourner Apr 2, 2021

zarov Apr 2, 2021 •

edited

Loading

mourner Apr 2, 2021

zarov Apr 2, 2021

mourner Apr 5, 2021

mourner Apr 5, 2021

zarov Apr 5, 2021

mourner Apr 5, 2021

zarov Apr 5, 2021

zarov Apr 5, 2021

zarov Apr 5, 2021

ghost commented Apr 7, 2021

Batch render DOM elements to avoid reflow #10530

Batch render DOM elements to avoid reflow #10530

Conversation

zarov commented Apr 1, 2021 • edited Loading

Performance

Before

After

Launch Checklist

CLAassistant commented Apr 1, 2021 • edited Loading

mourner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarov Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 7, 2021

zarov commented Apr 1, 2021 •

edited

Loading

CLAassistant commented Apr 1, 2021 •

edited

Loading

zarov Apr 2, 2021 •

edited

Loading