alankaanil commented 5 years ago

mapbox-gl-js version: v1.3.1

browser: Safari for iPhone / Macbook

I am using a single HTML page with example provided in Mapbox-gl-js page.

Steps to Trigger Behavior

Open the web page on Safari browser
Progressively Zoom out to maximum level, then zoom in back to initial level.
Memory keeps increasing each time Step 2 is repeated, but it does not go down.

Link to Demonstration

https://codepen.io/anilalanka/pen/pozxPGx Please add your access token for testing.

<!DOCTYPE html>
<html>
<head>
    <meta charset='utf-8' />
    <title>Display a map</title>
    <meta name='viewport' content='initial-scale=1,maximum-scale=1,user-scalable=no' />
    <script src='https://api.tiles.mapbox.com/mapbox-gl-js/v1.3.1/mapbox-gl.js'></script>
    <link href='https://api.tiles.mapbox.com/mapbox-gl-js/v1.3.1/mapbox-gl.css' rel='stylesheet' />
    <style>
        body { margin:0; padding:0; }
        #map { position:absolute; top:0; bottom:0; width:100%; }
    </style>
</head>
<body>

<div id='map'></div>
<script>
    mapboxgl.accessToken = 'your-access-token';
    var map = new mapboxgl.Map({
        container: 'map', // container id
        style: 'mapbox://styles/mapbox/streets-v11', // stylesheet location
        center: [-74.50, 40], // starting position [lng, lat]
        zoom: 9 // starting zoom
    });
</script>

</body>
</html>

Actual Behavior

The following are the Memory profile details captured on Macbook Safari Web inspector, based on style URL used.

When memory consumed is large enough,

Macbook Safari browser throws a warning saying This webpage is using significant memory. Closing it may improve the responsiveness of your Mac..
iPhone Safari app refreshes the page.

1. Using Mapbox default style URL:

Memory increases upto 1.06 GB in 80 seconds, as shown below Screen Shot 2019-09-18 at 4 01 30 PM Screen Shot 2019-09-18 at 4 02 11 PM

2. Using our Product's custom style URL:

A faster rate of memory increase is observed - 1.76 GB in 40 seconds Screen Shot 2019-09-18 at 3 46 42 PM Screen Shot 2019-09-18 at 3 47 14 PM

On further investigation, I found the below tickets already present on Safari:

The last notable comment from Safari noted that "I checked further. It seems crash I am seeing does not have to do this with specific bug which indeed seems fixed. It seems Safari has some general limit of overall memory use and we are running into that due to memory bloat."

asheemmamoowala commented 5 years ago

@alankaanil26 Thank you for the details in this ticket.

Adding some notes from the tickets you posted: At https://bugs.webkit.org/show_bug.cgi?id=172790#c30, it states that:

there are thousands of smaller buffers, from approx 200k to 0 that are never garbage collected. They are all marked as root objects, in that they are hanging off the window.

Using the heap profiler/snapshot, it looks like the buffers come from:

GridIndex

Placement.retainedQueryData

Map.style

image.data

and a few other places.

The same thing happens in Google Chrome - which suggests it's probably an issue in MapBox.

We are investigating this and will post back here when we find the root cause.

zhenyanghua commented 5 years ago

It also happens on both Chrome and Safari in iOS when interacting with the map instance from the mapboxgl, e.g. when map extent changes, Tested the mapboxgl-js 1.3.1 on iOS 12.4.1 and Chrome 77.

iamanvesh commented 5 years ago

hi @asheemmamoowala any update on the issue?

asheemmamoowala commented 5 years ago

@iamanvesh We are still investigating the issue, and will post updates here when we have some concrete leads.

ezraripps commented 5 years ago

Same issue!

ezraripps commented 5 years ago

https://www.bountysource.com/issues/80631646-memory-keeps-increasing-with-each-zoom-interaction-on-iphone-macbook-safari-browser if anyone is interested

asheemmamoowala commented 5 years ago

We have found a couple issues that have been addressed in #8813 and #8814. The work on these PRs is still in progress, but any feedback you could provide by testing those PRs would also help confirm if the fix goes far enough

ezraripps commented 5 years ago

8814 did not fix so far, #8813 is related to .remove() so its not the same issue

ezraripps commented 5 years ago

Same issue for chrome on osx

ezraripps commented 5 years ago

setting maxTileCacheSize to 0 helped me

kkaefer commented 5 years ago

I've done some more profiling on macOS with https://github.com/mapbox/mapbox-gl-js/pull/8814 applied and did memory profiles of controlled zooming in/out with maxTileCacheSize set to 0.

Chrome 77

memory-chrome

Firefox 70

memory-firefox

Safari 13

memory-safari

It looks like this problem is isolated to Safari. What's weird is that the memory seems to increase in a terraced way. Possibly this is some internal vector that uses an allocation strategy that doubles the memory.

kkaefer commented 5 years ago

I ran another test with a debug build of Chromium 75 and a testing script that executes a flyTo to a random location every couple of seconds. After 1h30m, the master branch was at ~2GB and growing. With https://github.com/mapbox/mapbox-gl-js/pull/8814 applied, it remained stable between 400 and 500MB, with a peak of 659.7MB after ~1h40m.

arindam1993 commented 5 years ago

This is looking like a browser bug, after instrumenting with Xcode instruments and a debug build of WebKIt @ahk and I discovered that calls to WebGLRenderingContext.deleteBuffer() weren't freeing up any allocated memory.

We've come up with a simple example that reproduces the issue, and filed a bug with webkit. https://bugs.webkit.org/show_bug.cgi?id=203650

The example:

<html>
    <head>
        <title> Safari WebGL buffer leak</title>
    </head>
    <body>
        <canvas width=800 height=800 id='test'></canvas>
        <script>
            const canvas = document.getElementById('test');
            const gl = canvas.getContext('webgl');
            let vertBuffer = null;
            let indexBuffer = null;

            const size = 1e6 * 3;
            const vertexArray = new Float32Array(size);
            const indexArray = new Uint16Array(size);
            function createGlBuffers(){
                vertBuffer = gl.createBuffer();
                gl.bindBuffer(gl.ARRAY_BUFFER, vertBuffer);
                gl.bufferData(gl.ARRAY_BUFFER, vertexArray, gl.STATIC_DRAW);
                indexBuffer = gl.createBuffer();
                gl.bindBuffer(gl.ELEMENT_ARRAY_BUFFER, indexBuffer);
                gl.bufferData(gl.ELEMENT_ARRAY_BUFFER, indexArray, gl.STATIC_DRAW);
            }

            function deleteGlBuffer(){
                if(vertBuffer){
                    gl.deleteBuffer(vertBuffer);
                    delete vertBuffer;
                }

                if(indexBuffer){
                    gl.deleteBuffer(indexBuffer);
                    delete indexBuffer;
                }
            }

            function everyFrame(){
                deleteGlBuffer();
                createGlBuffers();
                window.requestAnimationFrame(everyFrame);
            }
            everyFrame();

        </script>
    </body>
</html>

kkaefer commented 5 years ago

A partial fix for a Safari bug related to CacheStorage is in https://github.com/mapbox/mapbox-gl-js/pull/8956. With that fix applied, memory growth is significantly reduced, but it's still growing over time.

I also discovered another Safari memory leak related to transferring buffers through message channels and ticketed it in https://bugs.webkit.org/show_bug.cgi?id=203990. I believe the remaining memory growth that the above PR doesn't fix can be attributed to the same or a similar problem, but so far we haven't been able to identify a suitable fix for Safari.

kkaefer commented 5 years ago

I'm using this demo page to trigger rapid memory growth:

<!DOCTYPE html>
<html>
<head>
    <title>Mapbox GL JS debug page</title>
    <meta charset='utf-8'>
    <meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
    <link rel='stylesheet' href='../dist/mapbox-gl.css' />
    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
    </style>
</head>

<body>
<div id='map'></div>

<script src='../dist/mapbox-gl-dev.js'></script>
<script src='access_token_generated.js'></script>
<script>

let zoom = 15;
const center = [-77.01866, 38.888];
const duration = 60000;

let map = window.map = new mapboxgl.Map({
    container: 'map',
    zoom,
    center,
    style: 'mapbox://styles/mapbox/streets-v10',
    interactive: false,
    maxTileCacheSize: 0
});

const interval = setInterval(() => {
    map.style.sourceCaches.composite.clearTiles();
    map.jumpTo({});
}, 1000);

</script>
</body>
</html>

The advantage over zooming/panning is that the tiles loaded are known, so there's less variation and it's easier to spot memory growth.

kkaefer commented 5 years ago

I've been debugging this further and will post a little bit about the progress, process and tools used.

Capturing memory use isn't straightforward; there are [a few different metrics](). I've settled for using "Real Memory Size" since it most closely reflects the actual memory use. Activity Tracker also shows it in the Info window for a given process. The number in the table overview seems to be a rolling average, so take it with a grain of salt.

To create profiles like those posted above, I'm using the Python module memory_profiler (install with pip). The default CLI program mprof doesn't allow attaching to existing processes. However, Safari creates separate rendering processes for every tab you open. Therefore, I created a small derivative script to record the memory use of an existing process by PID.

To visualize the memory trace that is currently in progress, I stripped down the mprof CLI tool and added auto-update support, resulting in a graph that interactively updates every few seconds.

My workflow is to open the debugging site posted above in a new window/tab, then use Activity Monitor (or a CLI tool) to find the correct process. In Activity Monitor, the process helpfully is titled with http://localhost:9966; copy the PID and use it to record the memory trace.

You can also use vmmap <PID>, but the majority of the memory is shown as "WebKit Malloc" and "DefaultMallocZone" zones. vmmap shows significant growth in the WebAssembly memory and the WebKit Malloc region.

The WebKit wiki contains an info page on Leaks and Bloats which links to a patch that adds many more zones to the malloc tracker. I applied the patch to a June revision of WebKit, compiled a release build and ran it with version 87 of the Safari Technology Preview.app (Safari 13 will fail to launch with older WebKit builds). Note that you'll have to make sure that the DYLD_FRAMEWORK_PATH includes both the build directory and the app's Framework directory separated by colons, otherwise it will default to using the wrong libraries. With that patch applied, vmmap shows many more regions as promised, but the memory growth we observe with Mapbox GL is still attributed the blanket WebKit Malloc zone.

kkaefer commented 5 years ago

I've tested memory growth with the tooling I described above with all GL JS versions back to 0.43 from late 2017, with the patch in https://github.com/mapbox/mapbox-gl-js/pull/8956 applied where applicable, but got the same memory growth, which looks similar to this (~30 minute trace):

out

It typically grows pretty quickly, up until 3-5 GB, then hovers around that area for a long time, sometimes reducing memory usage after a while. The initial memory use is often terraced, with big allocations happening in a very short time, then remaining completely stable for a few minutes.

I found that this erratic memory growth is connected to WebWorkers. I produced a build of GL JS that doesn't use them and instead processes all tiles on the main thread. It loads the same amount of tiles so the numbers are comparable. This is the memory graph I got in a ~50 minute trace:

out

To me, this indicates that Safari is capable of running GL JS without excessive memory growth, but that there's some bug or adverse behavior in conjunction with webworkers.

kkaefer commented 5 years ago

To simplify debugging with webworkers and making it easier to set breakpoints, I created this rollup script based on our CSP build:

import {plugins} from './build/rollup_plugins';

const config = (input, file, format) => ({
    input,
    output: {
        name: 'mapboxgl',
        file,
        format,
        sourcemap: 'inline',
        indent: false
    },
    treeshake: false,
    plugins: plugins(false, false)
});

export default [
    config('src/index.js', 'dist/mapbox-gl-main.dev.js', 'umd'),
    config('src/source/worker.js', 'dist/mapbox-gl-worker.dev.js', 'iife')
];

Running it with npx rollup -c rollup.config.debug-worker.js --watch produces a separate main and worker bundle. To use it, replace the script inclusion with

<script src='../dist/mapbox-gl-main.dev.js'></script>
<script>
mapboxgl.workerCount = 1;
mapboxgl.workerUrl = '/dist/mapbox-gl-worker.dev.js';
</script>

in your debug HTML page. Safari will now allow you to set a breakpoint in the worker bundle and correctly read the source maps. It has the added benefit that builds are faster, in particular if they only affect either the worker or the main bundle.

kkaefer commented 5 years ago

I've followed a number of different approaches, gradually disabling parts of the tile parsing on the worker side along the way. However, short of disabling creation of WorkerTile altogether, I've still been seeing memory growth.

Here's the graph when disabling passing data back to the main thread (but still parsing everything):

out

It's particularly noticeable that the memory stays flat for long time, then suddenly jumps very excessively. I've been monitoring the "Private Memory Size" in Activity Monitor and it seems that this is varying quite a bit, depending on when Safari runs the garbage collection. However, once Safari bumps against the memory ceiling, it looks like it won't reuse any of the already available space and allocates new space.

This is the graph with maybePrepare disabled (i.e. no symbol layout, and actually adding features to buckets, but with tile parsing):

out

It's still jumping quite a bit. When disabling tile parsing altogether (but still keeping WorkerTile creation), something interesting happens:

out

The steep cliffs become less steep. I'm explaining this with the fact that we don't create as many objects during one tile reload anymore on the worker.

kkaefer commented 5 years ago

We know that memory somehow keeps leaking in the web workers, since a build that runs on just the foreground thread doesn't exhibit memory growth. I produced an experimental build of GL JS that uses workers 10 times, and then explicitly terminates them after the last tile that was using the worker gets removed from the map. With the demo above, this is happening regularly and I checked that all workers are getting terminated after a few seconds. Yet, this is the memory graph I'm observing:

out

kkaefer commented 5 years ago

Another experiment I've done is to use MessageChannel, send the port to the webworker, and communicate via the channel instead of via the webworker's postMessage calls. However, the memory graph doesn't look stable either:

out

kkaefer commented 5 years ago

We've observed that this only happens when using web workers. We use web workers to load and process tiled map data for use with WebGL, then send it to the main thread. A typical tile can have up to a few hundred ArrayBuffers. We are using postMessage with an array of transferable objects to avoid buffer copies.

Furthermore, we've observed that disabling transferable objects mostly fixes the memory growth issue and the process stays at ~500-600 MB.

I've dug in to find out why this happens:

When sending a message to another thread with postMessage, WebKit creates a SerializedScriptValue object, passing in a list of transferable objects. The message gets serialized by the CloneSerializer, which adds indices to the ArrayBuffers stored directly in the SerializedScriptValue object.
The message with the SerializedScriptValue gets pushed to the worker's or main thread's queue. Once the event gets dispatched, a new MessageEvent object is created, which obtains ownership of the SerializedScriptValue.
WebKit doesn't seem to garbage collect MessageEvent objects frequently enough. I instrumented SerializedScriptValue and found that it can accumulate several thousand of those objects until GC kicks in, causing the process to allocate more and more memory. This means it's not technically a leak, but still causes excessive memory growth.

A few other observations:

The MessageEvent has to actually be dispatched into JS land. If there's no event listener, the SerializedScriptValues get destructed immediately when the message is processed on the other thread. This means that the mere creation of a MessageEvent doesn't cause the leak.
Memory grows regardless of whether the value is ever deserialized. I rebuilt WebKit with deserialization disabled entirely and still observed the memory growth caused by WebKit hanging on to MessageEvent objects.
While the memory growth is impacted by the byte count of the transferred ArrayBuffers, the number of elements in the transferables list has at least an equal effect. I created a small benchmark that allocates zero-size ArrayBuffer, then adds the same object 8192 times to the list of transferables. This causes the vector allocated to hold the ArrayBufferContent objects to become pretty large even if all of its members are zero because WebKit filters duplicates.
When attaching a smallish ArrayBuffer to the MessageEvent object when it is received by just executing event.foo = new ArrayBuffer(32768), I was able to trigger GC of those MessageEvent objects much more frequently, mitigating the memory growth somewhat. This leads me to believe that GC isn't aware of the actual size of MessageEvent objects. It should be a combination of the byte size of the stored ArrayBuffers and the count of them.
I've used the GC Heap Inspector and it looks like the MessageEvent objects aren't referenced from a GC root.
The direction of transfer (worker → main, or main → worker) doesn't seem to matter for how frequently they are GCed.

I ticketed this over in https://bugs.webkit.org/show_bug.cgi?id=204515 along with a reproduction example.

asheemmamoowala commented 5 years ago

@alankaanil @zhenyanghua @ezraripps A fix for this is available in the 1.5.1-beta release.

ezraripps commented 5 years ago

@arindam1993 (or whoever did) hey thanks man, https://www.bountysource.com/issues/80631646-memory-keeps-increasing-with-each-zoom-interaction-on-iphone-macbook-safari-browser idk how but go claim this

maggo commented 4 years ago

It looks like this was not released with v1.6.0, or is it just missing from the changelog? Any ETA in that case?

Edit: Oops, sorry. Thought 1.5.1 was just a beta release. The PRs are in master and live with 1.6.0

kkaefer commented 4 years ago

1.5.1 is also a final release which includes this fix. It's also available in 1.6.0.

hallahan commented 2 years ago

It looks like this bug was fixed by Apple on June 4, 2020.

Should this issue be re-opened? It would be nice to turn back on transferrables in Safari.

https://trac.webkit.org/changeset/262581/webkit

mapbox / mapbox-gl-js

Memory keeps increasing with each zoom interaction on iPhone / Macbook Safari browser #8771

Steps to Trigger Behavior

Link to Demonstration

Actual Behavior

1. Using Mapbox default style URL:

2. Using our Product's custom style URL:

8814 did not fix so far, #8813 is related to .remove() so its not the same issue

Chrome 77

Firefox 70

Safari 13