FillBucket.uploadPending() is always true: `this.uploaded` is never set, so every fill bucket re-uploads on every frame

Open Beginner friendly
  • Is something wrong?
#13,752 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
82/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
typescript
Domain
performance

Research direction

Read the FillBucket.upload() and uploadPending() implementations, then compare them with LineBucket and CircleBucket as the issue describes. Reproduce by checking whether a loaded fill bucket remains pending after upload; confirm the fix by verifying it becomes non-pending when no re-upload is needed and that rendered colors remain unchanged.

Written by the indexing model from the issue text.

Description

bug :lady_beetle: performance :zap:
mapbox-gl-js version

3.33.0 and 3.34.2 (latest on npm; same code in both)

Browser

Chrome (desktop)

What happens

FillBucket.uploadPending() reads this.uploaded, but FillBucket.upload() never sets it. Only the inner FillBufferData sets its own uploaded flag. The bucket therefore reports "pending" forever.

From dist/mapbox-gl-dev.js (3.33.0):

// class FillBucket
uploadPending() {
  return !this.uploaded || this.bufferData.needsUpload() || this.hdExt != null && this.hdExt.needsUpload();
}
upload(context) {
  this.bufferData.upload(context);   // sets bufferData.uploaded, not this.uploaded
  if (this.hdExt) {
    this.hdExt.upload(context);
  }
}

Compare LineBucket and CircleBucket, whose upload() ends with this.uploaded = true.

Because the bucket is always pending, Tile.upload() takes its slow branch for every fill bucket on every call:

// Tile.upload
if (bucket.uploadPending()) {
  ...
  featureStates = sourceCache._state.getState(sourceLayerId, void 0);
  ...
  bucket.upload(context, this.tileID.canonical, featureStates, availableImages, globalProperties);
}

getState(sourceLayer, undefined) builds a new object with Object.assign({}, base, changes) over every feature that has state in that source layer. So the cost per frame is (visible fill tiles) x (features with feature-state), even when nothing changed.

Impact

On a map with a vector fill layer and many features carrying feature-state, the main thread spends most of a pan inside Tile.upload. Measured on a phone-size map (390x789, zoom 16) with roughly 5,000 to 7,000 features holding state, 7 pans over the same ground with the tile cache cleared first:

Tile.upload main thread over 7 pans long frames (> 34 ms) in the worst pan
unpatched 3,545 ms 129 ms
patched (below) 118 to 125 ms 0 to 40 ms

The cost grows with the number of features holding state. With few of them the bug is still there but cheap.

Steps to reproduce
  1. Add a vector source and a fill layer on it.
  2. Call map.setFeatureState for a few thousand features of that source layer.
  3. After the map is idle, take a fill bucket from a loaded tile and call bucket.uploadPending(). It returns true, and bucket.uploaded is never set.
  4. Pan, and profile Tile.prototype.upload: getState runs for every fill tile on every call.
Expected

uploadPending() returns false once the bucket has uploaded and nothing needs a re-upload.

Suggested fix

Set the flag in FillBucket.upload(), as the other buckets do:

upload(context) {
  this.bufferData.upload(context);
  if (this.hdExt) {
    this.hdExt.upload(context);
  }
  this.uploaded = true;
}

Or have uploadPending() read this.bufferData.uploaded.

Workaround

Wrap FillBucket.prototype.upload at runtime so it sets this.uploaded = true after the original call. Rendered colours were unchanged with the wrap in place.

Dominant language
TypeScript
Stars
12.5k
Forks
2.4k
No merged PRs in 30d

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from mapbox/mapbox-gl-js

All issues in mapbox/mapbox-gl-js

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.