Fix double counting of createImageBitmap requests in RequestScheduler #8163
Conversation
|
Thank you so much for the pull request @steve9164! I noticed this is your first pull request and I wanted to say welcome to the Cesium community! The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.
Reviewers, don't forget to make sure that:
|
|
@steve9164 is a member of my team and is covered by our CLA. |
|
Thanks for confirming that Kevin, we added Stephen to Data61's corporate CLA. |
|
@steve9164 please merge in master |
…sium into fix-double-counting-upstream
|
Merged! |
|
@tfili or @OmarShehata could you review? |
| Resource._Implementations.createImage = function(url, crossOrigin, deferred) { | ||
| expect(url.indexOf('.tif?cesium=true')).toBeGreaterThanOrEqualTo(0); | ||
| Resource._Implementations.createImage = function(request, crossOrigin, deferred) { | ||
| expect(request.url.indexOf('.tif?cesium=true')).toBeGreaterThanOrEqualTo(0); | ||
|
|
||
| // Just return any old image. | ||
| Resource._DefaultImplementations.createImage(imageUrl, crossOrigin, deferred); |
There was a problem hiding this comment.
Shouldn't all the Resource._DefaultImplementations.createImage calls in the tests now take request? There are a bunch spread throughout the tests.
There was a problem hiding this comment.
You're right.
|
I've fixed the tests and I think this is ready for another review. I think the CI error is not due to my changes, since it comes from |
|
Tests pass now and the new ES6 changes have been merged in. I think this is a pretty big issue. At the moment any Cesium app running on a version of Chrome that supports |
This is still true with this PR, it's just not counting them anymore, right? |
|
With this PR an image request will correctly be counted as 1 request (instead of 2), so Cesium's default request limit of 6 per domain will be correctly applied, instead of Cesium incorrectly limiting to 3 real requests. Here are some screenshots from Chrome running my changes and running the current Cesium. I set Chrome to throttle requests (added a 4000ms latency so that the number of in-flight requests was obvious), used Stamen Watercolour map (because it uses the On http://localhost:8080/Apps/CesiumViewer/index.html with my changes: On https://cesiumjs.org/Cesium/Apps/CesiumViewer/index.html: In the first there are 6 concurrent requests (which is correct) and the second screenshot shows only 3 concurrent requests due to the double counting issue. |
|
Thanks for the explanation @steve9164. I wasn't thinking about how overcounting was interfering with the request scheduler's throttling. We should definitely try to get this fix in this upcoming release since it sounds like this is slowing down imagery loading. |
|
Thanks again for your contribution @steve9164! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
2 similar comments
|
Thanks again for your contribution @steve9164! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
|
Thanks again for your contribution @steve9164! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
|
If I don't hear any complaints, I'm going to edit this lightly (e.g. sync with master) and then merge it. Probably today. It makes Cesium load imagery twice as fast. 🐎 |
|
Thanks @steve9164, sorry this took so long to merge! |
A potential fix to #8162.