mapbox / mapbox/mapbox-gl-draw
Poor CPU Performance: Keydown
Nobody has claimed this yet.
- 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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