Throttle the image request queue while the map is moving to improve performance #2097

zhangyiatmicrosoft · 2023-01-25T16:40:19Z

Launch Checklist

This is the same as #1856 which will be abandoned by AdamS108.
Need transfer the ownership over to me.

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Add an entry to CHANGELOG.md under the ## main section.

…erformance The image request queue can cause frame rate glitches while the map is moving. To mitigate this, allow the map renderer to throttle the amount of requests processed by the image request queue until the map stops moving again.

…-queue

…FT/throttle-image-request-queue

…-queue

…AGE_REQUESTS_PER_FRAME_WHILE_THROTTLED

…FT/throttle-image-request-queue

…-queue

…request_queue.ts.

…-queue

…a the pure function wrappers.

…-queue

…FT/throttle-image-request-queue

…age-request-queue Users/adam szofran msft/throttle image request queue

smellyshovel · 2023-01-27T06:44:00Z

@smellyshovel @zhangyiatmicrosoft is there a good way to solve this conflict? Will the split of the #2038 PR allow this to be merged in between the small PRs? Generally speaking, both PRs are in final implantation stages, but both need additional PRs to be fully complete (moving the queue to a different file in this PR and the split to smaller PRs in the other) If you guys want to review each other's code that can be helpful too.

Merge this one as it's ready. I think it's gonna be a long time until get to getImage in my PRs, so by the time I actually do, I will simply try to include the changes from here into my PR.

# Conflicts: # src/index.ts

zhangyiatmicrosoft · 2023-01-30T19:39:22Z

@HarelM consolidating all comments from the previous PR to keep us on the same page:

this.throttleControlCallbackHandleCounter++;
Q: Can this reach a number too big to become negative?
A: I don't think so. The design of this counter is to keep track of downstream consumers of ImageRequestQueue per session. It is practically impossible for the number to go beyond 2B.
HOWEVER, I did notice a bug when it comes to removeThrottleControlCallback, in that it was using splice and changed the index.
Will be fixed in next iteration.
Two places need strong type: will fix
"queue item knows about the queue"
I agree this is wrong and will fix.
Regarding the confusing usage of "this":
When rewind a little bit, I see class ImageRequestQueue is (and will always) be used as singleton and start to doubt why "class" is needed to begin with.
It was created based on your comment at Dev 22: "This file has all kind of variables laying around, any chance you can turn it into a class?"
While class nicely wraps things together, it introduces keyword "this" problem and does not minimize well.
Therefore, my proposal is change to "namespace ImageRequestQueue", removing the class.
Still wraps things nicely, with smaller runtime script, and avoid "this" keyword.
What do you think?

HarelM · 2023-01-30T20:08:36Z

Regarding 4 - the problem is not the usage of "this" it's the circular dependency. You should be able to write it down using a class or not, but I want this logic to be encapsulated nicely.

* Remove unused tile request cache (maplibre#2101) * Remove unused tile request cache * Remove the public method to clear the storage * Get rid of tile request cache's usages * merge 2101 * Combine `ResourceType` interface and const into a single enum (maplibre#2103) * Combine ResourceType interface and const into a single enum Move it from ajax.ts to request_manager.ts * Re-run render tests * Assign strting values to the enum * Revert changes to tests * Fix typo * Don't use MapLibre prefix; use const enum * Fix "npm run test-build" on Windows (maplibre#2106) * wip * fixed path * rename var * remove timeout * add type and use namespace * test passed * refactored and cleaned * min test * all working * restore min test * clean up the cancel function --------- Co-authored-by: Matthew Mamonov <g.smellyshovel@gmail.com>

…ft/my-maplibre-gl-js into merge-img-req-q # Conflicts: # src/source/vector_tile_source.ts # src/util/ajax.ts

# Conflicts: # src/source/image_source.ts # src/source/raster_dem_tile_source.ts # src/source/raster_tile_source.ts # src/style/load_sprite.ts # src/ui/map.ts

zhangyiatmicrosoft · 2023-02-01T19:52:30Z

New iteration, let’s start from the previous toipics

fixed
a new type “ImageRequestQueueItem”
"queue item knows about the queue"
The original code was somewhat in this style. The queue is self-driven, meaning as soon as one requested item is processed, it will move on. For this reason, it auto advances previously at ajas.ts, line 328 (advanceImageRequestQueue).
It is hard to see and honestly the original code was really difficult to understand as well.

In AdamS108’s iteration both Queue and Item are classes so it looked bad.
The real queue data structure is nothing more than an array. No separate class is needed.

In the latest iteration, “namespace ImageRequest”” wraps everything, and it should be much easier to read.
Fixed

More optimizations:

Finally moved to a separate file, so won’t have to deal with merge conflicts on ajax.ts. Everything gets moved around so code diff did not help much anyway.
Moved a few definitions around as well.
Generic code clean ups and shortened a few function/field names in favor of smaller scripts.
Happy to say because of all the above, release script size actually reduced ~800 bytes even though this PR is adding more logic.
Added a new file “imageRequest.test.ts”, in which first 211 lines are moved from “ajax.test.ts”, and added seven more UTs to cover everything in the new implementation.

src/ui/map.ts

src/util/imageRequest.ts

HarelM · 2023-02-01T22:12:59Z

Overall this is alot better.
There are some nested callback hell which I hope @smellyshovel can solve after this gets merged.
I've added some minor comments.
Not sure I fully see the value of a namespace over a class, and since namespaces are not currently in use in this project I'm not sure this is the right place to start...
But overall, very nice work.

…ft/my-maplibre-gl-js into merge-img-req-q

zhangyiatmicrosoft · 2023-02-02T05:16:25Z

.... Not sure I fully see the value of a namespace over a class

namespace generates an anonymous wrapper. Equivalent of a class with all static methods, and far more compact when it comes to minimized scripts.
I have seen other projects where "amd" is used in tsconfig which automatically wraps things of each file. It is certainly an option I can explore later for this project as well.
As it is now if not use namespace, everything would be just flat, which is not what you want either based on earlier discussions.

HarelM · 2023-02-02T08:16:21Z

THANKS!

Adam Szofran and others added 30 commits November 16, 2022 15:57

Add AJAX unit test.

8db9848

Fix cancelRequest function

d62505c

Merge branch 'main' into users/AdamSzofranMSFT/throttle-image-request…

0eada71

…-queue

lint fixes

c60a39a

Merge remote-tracking branch 'upstream/main' into users/AdamSzofranMS…

d134f91

…FT/throttle-image-request-queue

Merge branch 'main' into users/AdamSzofranMSFT/throttle-image-request…

33c9d76

…-queue

Merge branch 'main' into users/AdamSzofranMSFT/throttle-image-request…

5692024

…-queue

Fix typo in comment.

56a05ec

Make amount of throttling be configurable.

6237946

Rename MAX_PARALLEL_IMAGE_REQUESTS_WHILE_THROTTLED to MAX_PARALLEL_IM…

d0bc63d

…AGE_REQUESTS_PER_FRAME_WHILE_THROTTLED

Add getter/setter maxParallelImageRequestsPerFrameWhileThrottled()

15c1c76

Fix lint issue.

465503d

Fix math error when computing maxImageRequests.

1f997f4

Merge remote-tracking branch 'upstream/main' into users/AdamSzofranMS…

0fdd988

…FT/throttle-image-request-queue

Add changelog entry

3888071

Merge branch 'main' into users/AdamSzofranMSFT/throttle-image-request…

f4ab50d

…-queue

Move image request queue functions out of ajax.ts and into new image_…

7ccc3f1

…request_queue.ts.

Merge branch 'main' into users/AdamSzofranMSFT/throttle-image-request…

0b2e540

…-queue

Merge branch 'main' into users/AdamSzofranMSFT/throttle-image-request…

c075f6d

…-queue

Move image request queue code back into ajax.ts to minimize diffs.

9b0aab7

Reorganize image request queue code into a class (ImageRequestQueue).

d11c31a

Add jsdoc comments

dd31f69

Modify ImageRequestQueue class to call itself directly rather than vi…

3761165

…a the pure function wrappers.

Merge branch 'main' into users/AdamSzofranMSFT/throttle-image-request…

0f8db86

…-queue

Remove one instance of theQueue syntax that wasn't necessary.

35bc1a2

Fix unit test failure.

e1f0a2d

Merge remote-tracking branch 'maplibre/main' into users/AdamSzofranMS…

b2c178e

…FT/throttle-image-request-queue

Merge pull request #1 from AdamS108/users/AdamSzofranMSFT/throttle-im…

4b2bddd

…age-request-queue Users/adam szofran msft/throttle image request queue

Update changelog with PR 2097

f3b1f71

zhangyiatmicrosoft added 2 commits January 27, 2023 12:04

Merge branch 'main' into merge-img-req-q

451ab2d

# Conflicts: # src/index.ts

merge 2101

ba77785

zhangyiatmicrosoft and others added 5 commits February 1, 2023 10:32

Merge branch 'merge-img-req-q' of https://github.com/zhangyiatmicroso…

7aaa73b

…ft/my-maplibre-gl-js into merge-img-req-q # Conflicts: # src/source/vector_tile_source.ts # src/util/ajax.ts

Merge branch 'main' into merge-img-req-q

0f0d892

# Conflicts: # src/source/image_source.ts # src/source/raster_dem_tile_source.ts # src/source/raster_tile_source.ts # src/style/load_sprite.ts # src/ui/map.ts

lint

242f946

update to 754099

fea700d

zhangyiatmicrosoft mentioned this pull request Feb 1, 2023

Refactor makeRequest, makeXMLHttpRequest and makeFetchRequest request to be promise-based #2121

Closed