mapbox/mapbox-gl-js

FeatureLevelRemove- and SourceLevelRemove-Benchmarks in remove_paint_state.js are exactly the same

Open

#10,049 opened on Oct 23, 2020

View on GitHub
 (0 comments) (0 reactions) (0 assignees)JavaScript (10,532 stars) (2,203 forks)batch import
good first issue

Description

Unless I'm missing something, both benchmarks were already the same when they were added in #7761 by @peterqliu :

https://github.com/mapbox/mapbox-gl-js/blob/ff8e087b6e7cc9fe63eb4809a0895b38029a67e2/bench/benchmarks/remove_paint_state.js#L88-L100 https://github.com/mapbox/mapbox-gl-js/blob/ff8e087b6e7cc9fe63eb4809a0895b38029a67e2/bench/benchmarks/remove_paint_state.js#L102-L114

I haven't used these APIs yet, but I believe the SourceLevelRemove should have used something like this?

// No loop
this.map.removeFeatureState({source: 'land'});

Maybe also source-property level like this?

// No loop
this.map.removeFeatureState({source: 'land'}, 'bench');

Contributor guide

Tech stack
javascript
Domain
performancetesting
Issue type
test
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
2
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
under 1 hour
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
stale
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
clear
Prerequisites
JavaScriptmapbox gl js API
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
70
Research direction
The issue identifies that the FeatureLevelRemove and SourceLevelRemove benchmarks in bench/benchmarks/remove paint state.js (lines 88-100 and 102-114) are identical, which is likely a copy paste error. The SourceLevelRemove benchmark should use `map.removeFeatureState({source: 'land'})` instead of looping over features. Review the mapbox gl js API documentation for `removeFeatureState` and consider whether a source property level removal (e.g., `map.removeFeatureState({source: 'land'}, 'bench')`) is more appropriate. The fix involves modifying the SourceLevelRemove benchmark function to replace the loop with the correct API call, ensuring it tests the intended functionality. After making the change, run the benchmarks to verify correctness.