mapbox / mapbox/mapbox-gl-draw

Poor CPU Performance: Keydown

Open
#888 2 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

performance
Dominant language
JavaScript
Stars
1.1k
Forks
612
Avg merge
8d 9h
Merged PRs (30d)
5

Description

**mapbox-gl-js version**: 0.54.0
**mapbox-gl-draw version**: 1.0.9

We're working on a gmaps to mapbox implementation and during our QA, we noticed that sometimes the "shift-click" multiple-vertices gets very slow. After some deeper investigation, it appears that there are `mousemove` and `keydown` handlers implemented by `mapbox-gl-draw` that eat an enormous amount of CPU time. We've noted:

[#760](https://github.com/mapbox/mapbox-gl-draw/issues/760) deals with `mousemove` so this should just cover the problematic `keydown` handler.

### Steps to Trigger Behavior
Keydown
1. Load map and add draw control
2. Click Map
3. Hold Shift
4. CPU usage is immediately at 30-100%
Minimal Reproduction Repo: https://github.com/mikeomeara1/mapbox-gl-draw-performance

### Remediation
**Caveat: We're not experts in this library - so everything below is just what we've been able to work out brute force style**
Implementing a simple throttle mechanism seems keep the CPU usage at a reasonable level (seemingly) without any negative side effects (again, understanding we're still testing):

**`events.js`**:
```
// NEW CODE
function debounce(cb, interval, immediate) {
var timeout;
return function() {
var context = this, args = arguments;
var later = function() {

timeout = null;
if (!immediate) cb.apply(context, args);
};

var callNow = immediate && !timeout;

clearTimeout(timeout);
timeout = setTimeout(later, interval);

if (callNow) cb.apply(context, args);
};
};
// END NEW CODE

addEventListeners: function() {
ctx.map.on('mousemove', events.mousemove);
ctx.map.on('mousedown', events.mousedown);
ctx.map.on('mouseup', events.mouseup);
ctx.map.on('data', events.data);

ctx.map.on('touchmove', events.touchmove);
ctx.map.on('touchstart', events.touchstart);
ctx.map.on('touchend', events.touchend);

ctx.container.addEventListener('mouseout', events.mouseout);

if (ctx.options.keybindings) {
ctx.container.addEventListener('keydown', debounce(events.keydown, 500)); // <-- HERE
ctx.container.addEventListener('keyup', events.keyup);
}
},
```
We would happily accept any feedback or optimizations on how to work around this!

Contributor guide

Open the contributing guide

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.

Research direction

Start with events.js, especially the keydown listener registered in addEventListeners, then run the linked minimal reproduction while holding Shift. Compare the handler's CPU usage with keybindings enabled and verify that the performance issue is resolved without breaking keydown and keyup behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
frontend, performance, web-dev
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.