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

Refactor loader #411

Merged
merged 8 commits into from Feb 21, 2019

Conversation

@jnordberg
Copy link
Contributor

commented Jan 10, 2019

Fixes #410

Also allows default trace and abort functions to use more efficient getString when available. Tests passing and should be backwards compatible.

@dcodeIO

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

While looking at this I figured that it totally makes sense to move the accessors to the memory instance, as it also avoids naming collisions with actual exports naturally. Might be worth to consider dropping backwards compatibility and update the documentation to the new semantics. Wdyt?

@dcodeIO

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Though, I believe that I had an issue once where it wasn't possible to extend a WebAssembly.Memory with new properties in some VM. Possible that my memory on this isn't correct or that this has been fixed already or something. Unsure.

@jnordberg

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

Yeah sure, sounds good. Might make sense to create a api compatible wrapper object for the memory instance instead of extending it to avoid any troubles.

@jnordberg jnordberg changed the title Move memory accessors from module to memory instance Refactor loader Jan 11, 2019

@jnordberg

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

Ported to TypeScript and extracted the helpers out from the module namespace into memory and table wrappers that does not modify the underlying WebAssembly.Memory and WebAssembly.Table instances. Should work in all VMs

@willemneal

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

This looks great! I had solved the issue by adding getStringImpl as an export, but this is a lot cleaner.

However, another needed feature for the loader has to do with running multithreaded code. I've been working on a library that implements the web worker API and one important aspect is that each instantiation of a module will copy the data section into memory, which can overwrite shared memory. Thus you need to first create a module with just the data to be instantiated and then use the exported memory for each of the subsequent modules. I had first solved this by hacking at the compile itself to not emit a data section. However, I think the best way to approach this is to use the parser to split a data section and module at load time. This is something that I'm also close to finishing, but am unsure where this all should take place. Your class abstraction might be the key; perhaps my threading library can just extend it to add this feature.

@dcodeIO

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Looking good so far! Code style should use semicolons, though, because that's essentially what's used everywhere else.

With no immediately useable index.js it might also make sense to separate the project into multiple subprojects first, like one for the compiler, one for the loader, one for the CLI and one for each example or something. Could use lerna or similar for that. Ideas on best practices in this regard?

@jnordberg

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

Semicolons added

I haven't used lerna before but seen it used in some projects and I like the structure. I think it could be a good option.

@dcodeIO

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

Or let's just add a new dist/assemblyscript-loader.js, ideally together with a .d.ts, that should do for now. Also, please add yourself to NOTICE before I merge :)

@dcodeIO dcodeIO self-requested a review Jan 24, 2019

jnordberg added some commits Jan 27, 2019

@jnordberg jnordberg force-pushed the jnordberg:loader-memory-accessors branch from 366a4ef to 5e2196d Feb 2, 2019

@dcodeIO dcodeIO merged commit bcaf5ef into AssemblyScript:master Feb 21, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@dcodeIO

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Sorry, took me a while to get to this, mostly because I'd like to actually play around with it a bit on top of master. Thanks! :)

@dcodeIO

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Hrm, actually, when I opened the loader source locally it instantly went all red due to code style issues (tslint vscode ext), like indentation, missing return types, single quoted strings, redundant public keywords. Really thought I had looked through this more thoroughly before, sorry. Going to revert for now. Let me know if you'd like to tackle those issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.