search

tangrams / tangram

WebGL map rendering engine for creative cartography
https://tangram.city
MIT License
2.22k stars 290 forks source link

calling scene.updateConfig causes memory consumption to increase #696

Closed rihteri closed 5 years ago

rihteri commented 5 years ago

TANGRAM VERSION: 0.16.1

ENVIRONMENT: Chrome 71

TO REPRODUCE THE ISSUE, FOLLOW THESE STEPS: Here is a very simple page that exhibits the issue:

<html>
    <div id="map"></div>
    <script src="https://unpkg.com/leaflet@1.3.1/dist/leaflet.js"></script>
    <script src="https://unpkg.com/tangram/dist/tangram.min.js"></script>

    <script>
        const map = L.map('map');
        const tangramLayer = Tangram.leafletLayer({ scene: {  } });

        tangramLayer.addTo(map);

        map.setView([0, 0], 12);

        window.setInterval(() => {
            if (tangramLayer.scene.initializing) return;

            tangramLayer.scene.updateConfig();
        }, 10);

    </script>
</html>

RESULT:

Included demonstration page starts with about 10 MB JS VM memory (according to chrome developer tools, memory tab). This doubles in about a minute or two and keeps climbing.

EXPECTED RESULT:

Memory consumption should remain somewhat constant even when reloading config.

POSSIBLE FIX? I think I have identified at least two places where unused objects are kept. First, in Style.setGeneration WorkerBroker.addTarget is called repeatedly with a changing main_thread_target. Another issue seems to be Style.replaceShaderBlock. There, scope gets pushed to this.shaders.block_scopes[key] but never removed.

I have tried adding the following code to src/style/style.js and after a short time of testing it seems to slow down the runaway memory consumption. Of course, I'm not quite confident this doesn't break something else. If it looks good, I can make a PR.

// to beginning of Style.setGeneration()
        if (Thread.is_main) {
            WorkerBroker.removeTarget(this.main_thread_target);
        }

// to Style.removeShaderBlock (key) 
        if (this.shaders.block_scopes) {
            this.shaders.block_scopes[key] = [];
        }
meetar commented 5 years ago

Behavior confirmed, thanks @rihteri, we'll take a look! (This is a great issue btw)

bcamper commented 5 years ago

Thanks for submitting this and the very helpful research! You accurately identified the root cause (WorkerBroker targets not being removed). Unfortunately, the fix is a bit more complicated; the changes you proposed did not work in many more "active" scene conditions, including on the bundled Tangram demo. Some background:

The WorkerBroker is used to allow cross-thread communication between main and worker threads. The "target" concept is used as a text identifier by which these threads can identify the object they want to send a message to on the other thread. The reason these are "scoped" by scene generation for styles, is that there is a series of calls back and forth between threads during the async process of tile building. Examples include for text label rasterization (which is done on the main thread, because it has access to the DOM Canvas, while the worker thread manages the rest of the tile building process), and line dash texture rendering (similarly, the worker thread knows it needs a certain dash texture, but needs to ask the main thread to render a dash texture for it).

Previously, the targets were not scope by scene generation, and just persisted by style (e.g. so the lines style would have a single target called Lines). This caused issues when scene.updateConfig() (or scene.load()) modified the underlying scene, while other prior tile operations might be still on-going, causing existing calls on the worker thread to sometimes reach the new, wrong object, before receiving a "cancel" event from the main thread. This could result in problems such as: map language is changed by updateConfig(), but one tile still builds with the old map language; or the shader structure is changed, and one or more tiles fails to properly render because it is referencing symbols that no longer exist (or not referencing newly required ones).

So the scope by scene generation was a simple attempt to ensure that only the correct, "new" style objects were being called on the main thread. It's not ideal or particularly elegant, and there may be a better solution.

I believe I've solved this issue in https://github.com/tangrams/tangram/compare/style-leak. The additional changes I've made to solve this issue are:

Lastly, the shader block scopes fix was indeed straightforward (https://github.com/tangrams/tangram/commit/939d5c88546558651ca63f43f3deb9847577a7a1), though I think unlikely to be a cause of noticeable leak (or at least, orders of magnitude less).

This is definitely an egregious leak that should be fixed, I also just wanted to clarify that while updating the style with updateConfig() every 10ms is a good and valid way to stress-test, it should not be considered as a pattern for normal use -- the updateConfig() method is usually meant for changes such as changing the map language, API key, or flipping other style-level flags. These are usually called infrequently, generally a handful of times (or none) over the course of a map session, and therefore I wouldn't expect this leak to show up in normal day to day use. Still should be fixed though! :)

rihteri commented 5 years ago

That was a very fast fix! Special thanks for the thorough explanation.

The fixes seems to work perfectly (as in zero increase in memory use) for the test case and also for a slightly more complex case which involves some updateConfig()s - unlike my proposed fix which indeed caused more problems than it solved.

bcamper commented 5 years ago

Released in https://github.com/tangrams/tangram/releases/tag/v0.17.0