-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make benchmarks navigable and add fps tests for setData #2327
Conversation
Is this good to merge? |
Not yet. I'll give this a review later today. |
map.addLayer({ | ||
'source': 'geojson', | ||
'id': 'geojson-polygon-fill' | ||
type: 'fill', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we could generalize our existing fps benchmark to capture this perf regression? (i.e. by measuring FPS over the set of tiles specified in Having fewer & robuster benchmarks will help us find future performance regressions. |
5fb0698
to
e2c2dd2
Compare
@lucaswoj - Looking into this more, what I had feels a bit past contrived. Limiting this PR to a simple restructuring of the benchmarks as I look into this some more. |
d6e1028
to
27b199e
Compare
}; | ||
|
||
map.repaint = true; | ||
measureFramerate(DURATION_MILLISECONDS, function(err, fps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you choose fps to measure setData
performance? Can we measure tile creation time directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I just saw where you wrote:
This adds to setData tests to the fps bench as well per chat. One for a large feature collection, for a small feature collection.
So sorry for misspeaking. I didn't mean to imply that we should test setData
within the FPS benchmark. These should be two separate benchmarks.
- the
fps
benchmark tests the fps across a variety of streets-v8 viewports (i.e. those incoords.js
) - the
set-data
benchmark tests the time from aGeoJSONSource#setData
call to the data being rendered on screen for different sized GeoJSONs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Breaking these up. I put them together as seeing the normal fps was helpful, but I agree this limits the FPS benchmark from getter better.
27b199e
to
3fb40b3
Compare
3fb40b3
to
471fae0
Compare
@lucaswoj - updated the PR to break setData and FPS apart. Also changed setData to track the average time it takes to load all the tiles after a setData. This should be a better metric for setData perf than fps, though time to load tiles could go down and fps be negatively impacted. I don't think we want to break this test into two files per: |
Having two scores in this benchmark is a non-starter for me. Breaking this up is very easy. You are measuring two separate things. |
500b70c
to
dd3b349
Compare
@lucaswoj updated to be two tests. |
Looks good 🚢 |
@lucaswoj & @ansis here is a benchmark showing fps degrading as the zoom level approaches 22 when setting data on a geojson source.
Would love feedback on how to make this better judge performance. FPS doesn't capture what is rendered on the canvas that well for instance.