Rewrite SharedArray to use goja.DynamicArray #1848
Conversation
This should make it considerably faster, but probably more importantly, a lot easier to reason about and also supporting all operations supported by a normal Array in JavaScript. From my quick testing it seems that now the difference even for 10 elements is within 5% CPU difference, which IMO is even within the margin of error. fixes #1820
Codecov Report
@@ Coverage Diff @@
## master #1848 +/- ##
==========================================
- Coverage 71.60% 71.58% -0.02%
==========================================
Files 179 179
Lines 13928 13955 +27
==========================================
+ Hits 9973 9990 +17
- Misses 3323 3329 +6
- Partials 632 636 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| return o; | ||
| }; | ||
| if goja.IsUndefined(val) { | ||
| return nil |
There was a problem hiding this comment.
Shouldn't this check be done first, before the s.freeze() call?
|
|
||
| // we specifically use JSON.parse to get the json to an object inside as otherwise we won't be | ||
| // able to freeze it as goja doesn't let us unless it is a pure goja object and this is the | ||
| // easiest way to get one. |
There was a problem hiding this comment.
I'm still wondering why we stringify the array elements and then parse them back on Get(). Attempting to freeze a non-object would simply return it as is, so the stringify/parse dance doesn't seem necessary and is probably a lot of overhead.
There was a problem hiding this comment.
This was more or less explored in dop251/goja#232 and the answer is that ... while for extremely simple values such as String and numbers it might be feasible ... for even Objects, it is likely that sharing them between goja Runtimes will lead to race conditions even if frozen.
Again the EcmaScript standard is meant for single-threaded operations ... everything in the standard (except the few Atomic and Shared stuff inside of it) is meant for only 1 thing operating on objects from it. So even if theoretically today goja could support it, tomorrow due to change that might break.
Additionally, if we don't serialize and then deserialize, we will need to actually check that things such as lambdas(which can reference outside objects inside of them) are not present. Proxies will also be problematic, as well as probably property methods and so on ... All of those things are not an issue as long as we serialize to JSON and back, because goja follows the EcmaScript standard and it actually specifically says that things like that don't get serialized 🎉 .
Again the "overhead" is minimal and IMO the possible complications far outweigh the amount of additional reasoning and code that will be needed for this to work. Also again ... telling people that their data will be serialized to JSON and back is a thing that they can reason about as well, whatever else we come up with ... likely won't be so easy to reason about and test with available tools.
There was a problem hiding this comment.
Thanks for explaining, that makes sense. I think I glossed over the initial discussion in that issue. It might be good to link to it in a comment as a future reference for why this is done.
This should make it considerably faster, but probably more importantly, a lot easier to reason about and also supporting all operations supported by a normal Array in JavaScript. From my quick testing it seems that now the difference even for 10 elements is within 5% CPU difference, which IMO is even within the margin of error. fixes #1820
This should make it considerably faster, but probably more importantly, a
lot easier to reason about and also supporting all operations supported
by a normal Array in JavaScript.
From my quick testing it seems that now the difference even for 10
elements is within 5% CPU difference, which IMO is even within the
margin of error.
fixes #1820