playcanvas / playcanvas-sync

Real-time synchronization of files between PlayCanvas and your local machine
https://playcanvas.com/
MIT License
75 stars 19 forks source link

Add AsyncPool that is used to limit the number of requests to a server. #58

Closed querielo closed 1 year ago

querielo commented 1 year ago

The PR adds AsyncPool that is used to limit the number of requests to a server and make 4 concurrent requests. For example, it is especially important if we want to reduce time that is required to upload files to Playcanvas Editor.

willeastcott commented 1 year ago

Thanks for putting this together. If the motivation for this PR is to reduce the time to upload files, do you have any stats about the improvements this PR brings?

yaustar commented 1 year ago

I will take a look once I have a few chunk of time this week. It would be great to get stats as Will mentioned

querielo commented 1 year ago

Thanks for putting this together. If the motivation for this PR is to reduce the time to upload files, do you have any stats about the improvements this PR brings?

Yes, right now, we have hundreds of files (bundles) that can be rebuild. We cannot simply combine them into one bundle by webpack/grunt/gulp/... because there are many global variables in legacy code that have to be defined in strict order. So, if we change something in structure of bundles a developer can spend up to 15 minutes to upload all bundles.

willeastcott commented 1 year ago

So, if we change something in structure of bundles a developer can spend up to 15 minutes to upload all bundles.

So what speed ups are you seeing with this PR? If it's ~15 minutes to upload without the PR, what is it with the PR?

querielo commented 1 year ago

So, if we change something in structure of bundles a developer can spend up to 15 minutes to upload all bundles.

So what speed ups are you seeing with this PR? If it's ~15 minutes to upload without the PR, what is it with the PR?

Yes. With PR it requires a few minutes instead of 15-20 minutes. Right now, we patch playcanvas-sync while npm install

yaustar commented 1 year ago

I've just tried running this from your branch @querielo and get the following error:

/Users/syau/Snapchat/Dev/querielo/playcanvas-sync/src/utils/common-utils.js:295
                asyncPools.set(key, new AsyncPool(concurrency, rateLimit));
                                        ^

ReferenceError: AsyncPool is not defined
    at new CUtils.getAsyncPool (/Users/syau/Snapchat/Dev/querielo/playcanvas-sync/src/utils/common-utils.js:295:41)
    at new OverwriteAllRemoteWithLocal (/Users/syau/Snapchat/Dev/querielo/playcanvas-sync/src/sync-commands/overwrite-all-remote-with-local.js:15:26)
    at cb (/Users/syau/Snapchat/Dev/querielo/playcanvas-sync/pcsync.js:92:16)
    at Object.wrapUserErrors (/Users/syau/Snapchat/Dev/querielo/playcanvas-sync/src/utils/common-utils.js:112:35)
    at /Users/syau/Snapchat/Dev/querielo/playcanvas-sync/src/sync-commands/sync-utils.js:51:24
    at Interface._onLine (node:readline:485:5)
    at Interface._line (node:readline:864:8)
    at Interface._ttyWrite (node:readline:1216:14)
    at ReadStream.onkeypress (node:readline:288:10)
    at ReadStream.emit (node:events:527:28)
querielo commented 1 year ago

Hello, @yaustar @willeastcott

It seems the PR is relevant again. Probably, someone might have tried to DDoS the Playcanvas Editor. And you set up yesterday request limits on Cloudfront based on user IP. I overlooked this (sorry), and inadvertently put myself under the strict rate limit as per https://developer.playcanvas.com/en/user-manual/api/#rate-limiting

Please consider this PR again 🙏

querielo commented 1 year ago

ReferenceError: AsyncPool is not defined

Fixed.

yaustar commented 1 year ago

@willeastcott or https://github.com/yak32 will have to take over review of the PR 😄