nodeca / pica

Resize image in browser with high quality and high speed
http://nodeca.github.io/pica/demo/
MIT License
3.76k stars 245 forks source link

Build object with information on how long main thread task takes. #208

Closed Giwayume closed 3 years ago

Giwayume commented 3 years ago

207

puzrin commented 3 years ago

IMO this will not work as you expect, because

https://github.com/nodeca/pica/blob/0a49552da95ad7f3e857bb5423c836532bdcbc55/index.js#L352

Tiles are processed via concurrency limiter. You will have to wrap more narrow part of code and sum all results. Anyway, i will keep this open as memo.

General plan is to cleanup garbage first, then decide how to arrange your request:

Some questions:

Giwayume commented 3 years ago

I'm using pica to redraw a scaled canvas to the resolution of the screen to make the pixel interpolation look nice. This happens on mouse wheel event. Though the pica call itself is being debounced before it takes effect (application-side), there's noticeable freeze on some device/browser combinations that occurs when getImageData() is called, especially when the canvas size is large such as 2000x4000.

Here's an example on the consuming application where I was testing the timing implementation: https://github.com/opengraphica/opengraphica/blob/master/src/ui/app-canvas.vue#L333

IMO this will not work as you expect, because Tiles are processed via concurrency limiter. You will have to wrap more narrow part of code and sum all results

It does seem to work in this case because the body of Promises execute immediately (synchronously). There's no code I see in the limit function that enforces an asynchronous wait such as setTimeout or requestAnimationFrame. So long as the promises themselves don't create async logic, they will all execute synchronously. In this case a resolved promise is returned before any async logic occurs.

Giwayume commented 3 years ago

I could be wrong and it's only measuring one iteration of the loop, invokeResize is the only part of that code I can find that runs async logic, so in theory the limiter will wait for that onmessage.

What I'm writing is a new application still in development, it's not urgent that it needs to be done in the next few days.

puzrin commented 3 years ago

https://github.com/nodeca/pica/blob/8df928c8be674634a6296def16f63657649e13d6/lib/utils.js#L44

Limiter's input is not promises, it's array of fabrics (functions) creating promises (each processing single tile). It creates new promises until concurrency limit reached, then stop until number of active promises reduced.

Seems you missed my second question:

How urgent is that? I'm busy with other projects

Giwayume commented 3 years ago

I said in last sentence it's not urgent, I'd be fine if you get to it within a month.

puzrin commented 3 years ago

https://github.com/nodeca/pica/blob/master/index.js#L265

Take a look at new code. You can wrap that method with your extra code to measure/collect time. Note, dist/ files were not updated, you will have to rebuild those.

Giwayume commented 3 years ago

I'll try it out

Giwayume commented 3 years ago

@puzrin for some reason the code on master seems to have worse performance overall in terms of hanging up the main thread, but yes the change you made was sufficient for me to be able to measure it.

Giwayume commented 3 years ago

I think the problem I'm seeing with performance is the cancelToken isn't working as well as before. If I fire off a bunch of pica.resize requests 100ms apart, but canceling each one each time so supposedly there should only be 1 running at a time, it's queuing up a bunch of main thread requests for work that shouldn't be processed anymore.

puzrin commented 3 years ago

@Giwayume i don't participate in perf discussions without correct reproducible benchmarks. Busy with another things. If initial issue was solved, please close. If another issue exists - create new ussue/pr with details.