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

Make objects only thread-safe when explicitly configured #341

Closed
gbrail opened this issue Sep 28, 2017 · 2 comments
Closed

Make objects only thread-safe when explicitly configured #341

gbrail opened this issue Sep 28, 2017 · 2 comments

Comments

@gbrail
Copy link
Collaborator

gbrail commented Sep 28, 2017

There is some machinery in ScriptableObject aimed at making individual objects in Rhino thread-safe. In working on the new hashing code to deal with hash collisions, I encountered this.

From inspecting the code in ScriptableObject, I'm not convinced that the assumptions made there about thread safety are correct. In particular, "get" operations on the object are unsynchronized -- we only synchronize when we modify an object. If Java had a real "race detector" like Go has I'm pretty sure that we'd pretty quickly find these issues.

However, there are "thread safety" tests in the test suite, and they do pass, so at least at the moment, individual JavaScript objects in Rhino are "mostly" thread-safe. Furthermore, I don't know if we have applications that use Rhino that depend on that fact.

Furthermore, I have tried removing all the synchronization from ScriptableObject and have seen that it has no measurable performance impact. (The "get" path is much much hotter than any other, and uncontended synchronization in Java is cheap.)

However, I have also tried various ways to make ScriptableObject more thread-safe (namely, synchronizing on modifications to an object) and I have seen that it DOES have a performance impact, and in fact a fairly big one. (So uncontended synchronization isn't THAT cheap.)

I propose the following -- that we add an option to Context named "THREAD_SAFE_OBJECTS." There will be two settings, and the default will be "false":

false (default): No synchronization at all in ScriptableObject. Applications should not share JavaScript objects between threads without using the "sync" Rhino extension. However, a single Rhino application may have many threads and Contexts as long as they are not shared between threads. (This is also how V8 and Nashorn work.)

true: All operations on ScriptableObject are protected by locks, including reads. All objects may be shared between threads. However, performance will be worse than we currently see in Rhino.

Finally, the "sync" function is still supported for applications that need actual control over the thread-safety of Rhino, as described here:

http://hns.github.io/2011/05/12/threads.html

I'd love to hear from people who rely on the fact that Rhino has thread-safe objects by default!

@gbrail
Copy link
Collaborator Author

gbrail commented Nov 14, 2017

This will be addressed in #344. Anyone interested in trying it out sooner or who has concerns should check out that PR.

@gbrail
Copy link
Collaborator Author

gbrail commented Nov 20, 2017

Pushed to master.

@gbrail gbrail closed this as completed Nov 20, 2017
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

No branches or pull requests

1 participant