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

WIP: Persistence #51

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

Conversation

fritzbauer
Copy link

@fritzbauer fritzbauer commented Feb 27, 2021

Please find my draft PR. Comments and refactoring still outstanding.
Should be working as WASM in the browser if you compile it with environment variables set GOOS="js" and GOARCH="wasm".
The params for the DBs need to be supplied as Uint8Array:
`importScripts('/assets/sql-wasm.js','/assets/wasm_exec.js');

initSqlJs({locateFile: file => /assets/${file}}).then((SQL) => {
global._go_sqlite = SQL;
const go = new Go();
WebAssembly.instantiateStreaming(fetch("/assets/go-jwlm.wasm"), go.importObject).then((result) => {
go.run(result.instance);
});
});

var merged = mergeJs(masterData, slaveData, "merged.jwlibrary");
`

Comment on lines 37 to 38
//https://blog.twitch.tv/de-de/2019/04/10/go-memory-ballast-how-i-learnt-to-stop-worrying-and-love-the-heap-26c2462549a2/
ballast := make([]byte, 100<<20) //100MiB
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting article, but I don‘t really see a reason to do it. Go is in general quite good with GC and the article talks about a specific use-case. In the case of go-jwlm the complete process should only take a few seconds (if not even less) anyway, so I don‘t think that GC really „slows down“ the program. But maybe you can explain your thought behind it a bit? 😊

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I agree. Was adding this at some point to keep it for performance tests later. Also do not expect a huge benefit. Will do some cleanup, fix the build annotations and add some tests anyway.

@AndreasSko
Copy link
Owner

Thanks for the PR! :)

I have a few thoughts about the approach of having multiple FS implementations:
I‘m thinking about if it would make more sense to completely remove all the file-handling within Database.go and let it just handle the actual DB part: So, for example, the import/export function would look something like this:

func (db *Database) ImportJWLBackup(sql *sql.DB) error
func (db *Database) ExportJWLBackup(sql *sql.DB) error 

Another „Persistence“ package (or some other name) would handle things like unzipping, opening the SQLite DB etc. In this package we could have different FS implementations: So the normal one and the in-memory. The import/export process would then be handled by the persistance-package which actually uses the Database.go to run import/export. This would also make it easier to extend go-jwlm with another FS in the future. And it feels way clearer as one package only does one thing. If I find the time I might open a proof-of-concept PR to illustrate it a bit using the current FS implementation.

Another question I had: Is there a specific reason why you implemented the in-memory FS on your own instead of using something like https://github.com/blang/vfs? I don‘t necessarily have a preference for either, I was just wondering 😊

Either way: Nice work! I think it is good to think about this, so go-jwlm can be used with WASM in the future. This is definitely a good start. Thank you again! 😊

@fritzbauer
Copy link
Author

Agree it might be the cleanest to place the logic out of Database.go. Did not want to do too many changes to your code that's pretty much how I came up with this persistence (also not sure of an appropriate name).

Regarding vfs: Short answer, I did not find VFS when searching for this and I did not think it would become this much logic at the end.
In my first tests I was fiddling around with BrowserFS according to this guide to get it running without any changes to your code: https://github.com/wcchoi/go-wasm-pdfcpu/blob/master/article.md
However, I did not like patching the wasm_exec.js which is provided by Go.
I found this fs lib: https://github.com/spf13/afero however this looked too big to me and I was not sure if this will compile to wasm and if it allows to run completely in memory or requires any other libs which are not available in wasm. And I did the most common mistake: I thought it will be simple..."just a map...". Working on the details it ended up to be some mocked FS logic...
VFS looks really good, no objections to use it!

Personally I will spend some time in performance investigation now since there is some working PoC...it is a lot slower in the browser than the native version and on the phone it becomes even worse (even got a memory allocation panic on the phone). I think this will be my long-term project to replace the sql-statement-logic with the go-jwlm backend, once wasm performance is acceptable.

Also would love to compile mattn/go-sqlite3 or anything else to wasm, However, as of today cgo does not support wasm. Therefore, right now we are copying the data 5 times back and forth: go-jwlm <->js <-> sql.js

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.

2 participants