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

Compile sql.js to wasm instead of "just" asm.js #255

Merged
merged 48 commits into from
Apr 29, 2019
Merged

Conversation

Taytay
Copy link
Contributor

@Taytay Taytay commented Nov 2, 2018

Emits WebAssembly and asm.js targets.
Upgrades to Version 3.28.0 of Sqlite
Upgrades to Version 1.38.30 of Emscripten

I wanted to use the built-in modularize code, but I ran into some of the issues spelled out in emscripten-core/emscripten#5820, so I have continued the sql.js tradition of modularizing it manually via pre and post files. There is now a promise exported that you wait on, and this promise resolves with the SQL module. This way, SQL isn't usable until the WebAssembly or asm.js has been loaded.

Since I made some more fundamental changes to how the module is loaded, and since this project now emits so many targets, I am not sure if it would be better to fork sql.js?
sql.js is no longer drop-in compatible with previous versions due to the async loading. I have documented this here in README.md: https://github.com/kripken/sql.js/pull/255/files#diff-04c6e90faac2675aa89e2176d2eec7d8

I also ran into a bug involving the creation of the sql-optimized file, so I restructured the Makefile a fair amount. It's not as DRY/fancy now, but it was easier to fix the issue. I have also made it easier to update the version of Sqlite used when compiling by borrowing heavily from https://github.com/mandel59/sqlite-wasm

It's worth mentioning that the worker test running in the node environment does not work when loaded with a WebWorker simulator, so I have skipped that test for now.

This PR cleans up other issues as well:
Fixes #219
Fixes #173
Fixes #262
Fixes #258
Fixes #251
Fixes #232
Fixes #210
Fixes #115

I look forward to hearing your thoughts.

TODOs:

  • Ensure that the mechanism for loading the module asynchronously works well in the browser
  • Update the readme for browser and Node usage
  • Add myself to AUTHORS
  • Bump major version

If we compile without WASM, loading it's synchronous.
If we compile with WASM on, loading is async.

But we need a mechanism we can rely upon to tell us when the loading is complete, regardless of the underlying loading code.
So, we now have a "ready" promise that we can listen for on the exported module.

Also
This allows for `npm run test debug` to work and makes it easier to debug the worker.
Make the worker implementation respect the fact that the loading might be async
Switch to tiny-worker because it is more actively maintained
There was an error in the way the optimized file was being made that caused certain parameters not to be passed to emcc.
This new file isn't as DRY, but it is a bit easier to grok and debug
(All tests pass)
Adding .wasm files
"zero-three" does nothing. "Oh-three" is a valid option.
This meant that our optimized output files were _way_ too big.
* fix/async_loading:
  Add extra test scripts to make it more obvious how to test the various compiled scripts
  Replace invalid "-03" option with "-O3"

As part of the merge, I am removing the new test-memory-growth and am recompiling with the new -O3 setting in the makefile.

# Conflicts:
#	Makefile
#	js/sql-memory-growth.js
#	js/sql.js
#	js/worker.sql.js
No more "*-raw.wasm" files. They were somewhat confusing since sql.js loaded sql-raw.wasm rather than simply sql.wasm.
The optimized build now runs closure on the sql.js loader. (The pre and post wrappers don't have closure applied).
This results in quite a few output files, and could likely be cleaned up
@mikeptweet
Copy link

Would it be possible to use/test with the latest sqlite version?

@Taytay
Copy link
Contributor Author

Taytay commented Nov 2, 2018

@mikeptweet, I certainly could. I am already a bit nervous about the sheer number of things I'm recommending change, but I could create another branch with this. If I make the Makefile improvements I'd like to (essentially copying @mandel59's work here https://github.com/mandel59/sqlite-wasm), this will get even easier.

Use allocateUTF8StringOnStack instead
* fix/async_loading:
  Commit the latest compiled assets
  Don't use writeStringToMemory
(It doesn't due to the bug in node/worker detection though)
* fix/async_loading:
  Adding the load_sql_file.js that the tests depend upon
  The worker test would theoretically work if run by itself
  The test files now work correctly if run individually
@danielbarela
Copy link

Is there any status update to this pull request? Since this seems to be the long pole pull request blocking #260 I was hoping we could update the build flags to include -s NODEJS_CATCH_EXIT=0 which will fix the issue brought up in #173 and #262

@Taytay
Copy link
Contributor Author

Taytay commented Mar 12, 2019

Hey @danielbarela - I haven't had any "evening-time" to work on this PR since I last committed. It's quite close, but not at 100%. At this point, I think we should leave out any other changes and just try to get it finished. It needs some simple things like documentation in the README to show how it works, and I might change the name of the main class back to SQL to maintain compatibility. I'm not sure if anyone wants to pick up the torch to finish cleaning this up or tell me what else needs to be cleaned up, but I certainly wouldn't take offense.

These are my last real TODOs from a previous comment:

At this point, my main TODOs before merging are around ensuring that this library is usable not just in Node (CommonJS), but in the browser as well. I've updated the README, but it still needs notes about the breaking changes between this 1.0+ version and previous versions.

I took many of these examples from the gh-pages branch.
These examples are useful even outside of a Github Pages context, so I wanted to include them here. This makes it easier for the README to reference them.
Since they are now in the main branch, I have taken care to point to CDN libraries where I could so that it doesn't get cluttered with vendor code, like for the codemirror library.
Explain how to upgrade.
Explain how to distribute wasm files.
Update sample code.
@Taytay
Copy link
Contributor Author

Taytay commented Apr 25, 2019

Okay - I had another go at it, fixed bugs with the worker, added examples in code and the reader, and might get more time tomorrow. @lovasoa, let me know if you have any thoughts.

@lovasoa
Copy link
Member

lovasoa commented Apr 26, 2019

Thank you for your work @Taytay. I'll have a look at it as soon as I can.

Fixes kripken/sql.js/sql-js#173
Fixes kripken/sql.js/sql-js#262
…apper

This is slow and somewhat error prone since it results in recompiling the .wasm files, so I am leaving it out for now.
Also including some unused changes to test-worker.js that I added when trying to get that test running again.
Download sqlite on demand when building.
Remove old unused static sqlite source files.
No longer use keyword "nothing" in examples or tests because it leads to a SQL error. (NOTHING is now a reserved word in Sqlite)
@nullrocket
Copy link

Excited to see this, I have been using this branch in our beta for a while with a few tiny JS api patches. Thanks for the hard work, I look forward to contributing more actively.

@Taytay
Copy link
Contributor Author

Taytay commented Apr 26, 2019

@nullrocket : Thanks! I was excited to try and wrap this up. Any chance you could point me to your patches or even lightly explain them so I could see if they would be helpful to incorporate in another PR?

@Taytay
Copy link
Contributor Author

Taytay commented Apr 26, 2019

@lovasoa : I believe that this is ready for a final review. I edited the original text of the PR to describe it more accurately. I also added more commits today to upgrade to the latest SQLite and Emscripten, as well as fix some other issues.

@nullrocket
Copy link

@Taytay My patches were just a bit of wrapping around the api in the same spirit as these https://github.com/kripken/sql.js/issues/91 probably nothing useful at this point or very well thought out.

Once this lands I will probably work towards wrapping the API to match at least the features I need from https://github.com/journeyapps/node-sqlcipher This is not super useful to most and way beyond the scope of sql.js, but the work of implementing some of the codecs and extensions out there like https://www.sqlite.org/loadext.html will probably be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment