Skip to content
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

add $/async and $/await to racketscript/interop #293

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

stchang
Copy link
Member

@stchang stchang commented Jan 7, 2022

No description provided.

@stchang stchang marked this pull request as draft January 7, 2022 00:10
@stchang
Copy link
Member Author

stchang commented Jan 7, 2022

closes #253

@stchang
Copy link
Member Author

stchang commented Jan 7, 2022

Also addresses: #84

@stchang
Copy link
Member Author

stchang commented Jan 7, 2022

@gamburgm @vishesh I'm having problems making bitmap/url in 2htdp/image fetch images asynchronously (images get rendered in wrong order, or something wrong with the image object where it cant find render function)

Any ideas? Do I need to change the big-bang loop to be more amenable to asynchronous execution?

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2022

Codecov Report

Merging #293 (cd1933f) into master (d2dc0ba) will increase coverage by 0.14%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
+ Coverage   68.16%   68.30%   +0.14%     
==========================================
  Files          48       48              
  Lines        5308     5354      +46     
==========================================
+ Hits         3618     3657      +39     
- Misses       1690     1697       +7     
Impacted Files Coverage Δ
...ript-compiler/racketscript/compiler/il-analyze.rkt 86.48% <57.14%> (-0.60%) ⬇️
...cript-compiler/racketscript/compiler/transform.rkt 95.00% <66.66%> (+0.04%) ⬆️
...cript-compiler/racketscript/compiler/assembler.rkt 95.54% <100.00%> (+0.09%) ⬆️
racketscript-compiler/racketscript/compiler/il.rkt 97.52% <100.00%> (+0.04%) ⬆️
racketscript-compiler/racketscript/interop.rkt 74.74% <100.00%> (+1.57%) ⬆️
...-compiler/racketscript/compiler/runtime/kernel.rkt 4.92% <0.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2dc0ba...cd1933f. Read the comment docs.

(lambda (resolve reject)
(define image (new #js*.Image))
(:= #js.image.crossOrigin #js"anonymous")
(:= #js.image.src (js-string data))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I'm understanding this correctly, this won't be using async/await. And I can see why this may not work, as when we render, the image may not be ready as its getting downloaded in background. I think this is HTMLImageElement and maybe you can add an assert for complete field in render() to see if we render once image is loaded.

(new (UrlBitmap url)))

;; doesnt work, "render" is not a function"
#;($/define/async (bitmap/url url)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably what we need to be doing, except I think bitmap/url is supposed to be a synchronous function, so this being async, will return aPromise object instead of Bitmap, and probably that's why it gives render is not a function error (since the contract is broken by it being a Promise).

Not sure what would be right way atm. Practically, its bad idea to put bitmap/url function in game-loop. It will block the frame until image is loaded in Racket too.

@gamburgm
Copy link
Contributor

gamburgm commented Jan 8, 2022

Unfortunately I think you'd have to fully commit to using async in big-bang; that is, you can't have individual 'renderable components' that are async, you want all of them to be.

.then prevents deterministic evaluation of promises unless you're able to chain them, which would mean CPS-ing a large chunk of image.rkt and universe.rkt, probably not ideal.

EDIT: .then, in this case, actually is deterministic -- promises are evaluated in FIFO order (once they start executing), which is why the nested place-image calls manage to render the images in reverse order.

The alternative is to make every render function async, and await every recursive render call. You can cut off the coloring effect in to-draw, and just call .then on the resulting promise from img.render with a thunk, or just leave it alone (I forget which is correct off the top of my head).

A little unfortunate though that you'd have to make such a significant change just to allow for async. For what it's worth, I'd guess that the async version is the more performant one in the typical case, since lots of promises (especially if they just get awaited) don't incur a large overhead and fetching images or handling other I/O in the background is a big win.

Let me know if the above makes any sense.

@stchang
Copy link
Member Author

stchang commented Jan 8, 2022

That's what I was afraid of. Makes sense though, thanks. I'll keep it in mind, in case someone is looking for a small project.

@stchang
Copy link
Member Author

stchang commented Jan 13, 2022

Just pushed something that seems to "work".

It makes the big-bang setup step async, and then doesnt call the start method until everything is ready.

I then added a ready method to the async imgs, which gets called in setup.

One weird thing is that I couldnt find a good place to save the async objs that need resolving so right now it's just a global array in jscommon.rkt. Is there a more idiomatic way to do this in js?

You can play around with it if you want:

If you compare the network tab in the browser for each one the async one seems a little faster, but I need to come up with some more stressful tests

@gamburgm
Copy link
Contributor

I think idiomatic javascript would look pretty different:

  • bitmap/url returns a promise
  • the functions that compose images (e.g. place-image) also return one promise that resolves the child promises
  • big-bang just awaits the one promise it needs to at the end

which gives the added benefit of not having to wait for images to load at the beginning that aren't needed until later (or needed at all).

I can't think of a better solution than this of the top of my head without coloring all the functions async.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants