corywalker / expreduce

An experimental computer algebra system written in Go
MIT License
381 stars 25 forks source link

Concurrent access fatality ;) #166

Closed justinclift closed 5 years ago

justinclift commented 5 years ago

Came across something interesting today, while experimenting with expreduce in a toy http server backend.

If several queries hit the server concurrently - causing multiple simultaneous calls to expreduce (one per request), the server immediately barfs with a fatal error:

fatal error: concurrent map writes

Using the Go race detector (go run -race main.go) on the code points to this global map being the problem:

// A nasty global to keep track of ToString functions. TODO: Fix this.
var toStringFns = make(map[string]ToStringFnType)

As a rough initial guess, the solution in my case will probably just be to serialise the expreduce operations. Toy server after all (for now). :wink:

But kind of wondering if there's a ETA for the "Fix this." bit of it's associated TODO comment? :smile:

corywalker commented 5 years ago

I can take a look at this to see if it's an easy fix.

BTW: It's interesting to see you've built this library into WASM. I've heard there were ongoing efforts to get Go working with WASM. IIRC there were some difficulties with getting GC working or something. I see you've also found that the performance is bad. If you want to help with the performance, I would suggest 2 things:

  1. Only call NewEvalState once if you can manage. You should be able to call it once when the server loads and share it for each request. This may be the reason for this bug report, though. I haven't tried concurrent Eval() calls to the same EvalState.
  2. When doing the plotting, use the Table[] function instead of setting a variable, calling the function, and repeating. Documentation here: https://corywalker.github.io/expreduce-docs/builtin/list/table/#table

Apologies for the possibly confusing interface. I've taken steps to document the functions within Expreduce but not the interface for the CAS.

justinclift commented 5 years ago

Thanks @corywalker, those tips will likely help. :smile:

One of the downsides with Go's Wasm implementation (for now) is that it bundles all of it's needed libraries into a single (huge) file. Adding expreduce bumps the size up (in my toy testing repo) from 8MB to 12MB.

The initial loading time for the wasm to be parsed (before starting) increases a lot too. I'm kind of thinking the client-server approach will be the right direction, so expreduce does it's thing on the server and the browser/client side just does the display and rendering. With that done, things are much, much faster to load and display content than with bundling expreduce into the wasm file itself.

That being said, my completely naive first-go implementation on the server is just copying the code sequence I saw when stepping (debug style) through the cli implementation. Definitely a "get it working first, make it pretty later" thing. :wink:

Your info above on how to do it the right way will improve the server side. :smile:

corywalker commented 5 years ago

I was able to remove the global map. Thanks for raising the issue. It should now be safe to create as many EvalState as you like and use them concurrently.

There is one thing I haven't tested yet though, and it may be an issue if you want to share a single EvalState for your server. This might create concurrent calls to Eval within a single EvalState and it might try to access a map concurrently. I can fix this with mutexes, and this will likely be next on my todo list.

corywalker commented 5 years ago

I could not trigger a similar issue while doing concurrent calls to Eval within a single EvalState. I'll assume it works for now. Feel free to try something similar in your code.

justinclift commented 5 years ago

Awesome. I'll give this a go (unsure if tomorrow or the weekend) and report back. :smile:

justinclift commented 5 years ago

Initial basic testing now, and the initially reported concurrency problem hasn't shown up yet again.

I'll do some more stuff with this tomorrow, and if weirdness does show up, I'll report back with the details. :smile:

corywalker commented 5 years ago

I'm glad the problem disappeared!