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

Relax multithreading requirements to allow multiple non-concurrent native threads #391

Closed
7 tasks done
svaarala opened this issue Oct 3, 2015 · 18 comments
Closed
7 tasks done
Labels
Milestone

Comments

@svaarala
Copy link
Owner

svaarala commented Oct 3, 2015

Current threading requirement is that only a single native thread can call into Duktape and another thread may enter only when the previous one exits.

When implementing blocking native calls it'd be useful to relax this so that:

  • Thread A enters by calling into a function (duk_call() etc).
  • Thread A calls a native function which, for example, sends a network request and then blocks waiting for a response (this may just be a blocking platform call or an explicit wait loop).
  • At this point thread A has called into Duktape but is not actively executing.
  • Thread B calls into Duktape.
  • Thread B exits from Duktape.
  • Thread A resumes.

Thread B could equally well also call into a blocking native function and allow thread C to execute while that happens. This policy requires that the user code manage and exclude multiple threads accurately, e.g. by using an "execution permission" lock. In the example above thread A would obtain the lock before executing, and release the lock just before blocking. Duktape API calls are only allowed when holding the lock. The policy also implies that the Duktape threads involved in threads A and B (etc) never overlap.

Implementing this policy needs some internal state changes so that thread A can resume even when something has happened in the middle. For example, current code assumes heap->curr_thread which needs to be saved and restored or removed entirely.

Tasks to implement first version into Duktape 2.0:

@svaarala
Copy link
Owner Author

svaarala commented Oct 3, 2015

Since the user code will in most cases need to manage some kind of execution lock, it'd probably be quite OK to require user code to use some API calls to tell Duktape when the thread may change and when thread A resumes.

@fatcerberus
Copy link
Contributor

So in this scenario, when Thread A resumes (the blocking call is satisfied), it would have to request the mutex back immediately?

@svaarala
Copy link
Owner Author

svaarala commented Oct 3, 2015

Yes, before making any Duktape API calls.

@svaarala
Copy link
Owner Author

svaarala commented Oct 3, 2015

But note that mutexes/locks are not required - the user code just has to guarantee that there are no simultaneous active calls into Duktape API. Mutexes/locks are probably the easiest way to do that, but some applications may be able to guarantee that without explicit locking too.

@fatcerberus
Copy link
Contributor

I was never daring enough to try lockless programming. :)

@svaarala
Copy link
Owner Author

svaarala commented Oct 3, 2015

Hehe, I was more referring to cases where the program is a single threaded event loop which executes a callback until it blocks - and then resumes to handle another event. In this case there's actually just one native thread which alternates between multiple active Duktape threads. Locking would not be necessary because there's just one native thread.

@andoma
Copy link
Contributor

andoma commented Oct 4, 2015

FTR, Spidermonkey supports this via the JS_SuspendRequest() and JS_ResumeRequest() API

@svaarala
Copy link
Owner Author

svaarala commented Oct 4, 2015

Hmm, that's closely related but seems to a little bit different thing (e.g. garbage collection is never locked/held in Duktape)? It's still probably good reference if coming from Spidermonkey.

@fatcerberus
Copy link
Contributor

Spidermonkey probably has to suspend GC in this case because it's done off-thread and may cause objects to be moved, potentially invalidating pointers in the process. Duktape actually has it a bit easier because it's single-threaded at the heap level and IIRC heap pointers are stable for the lifetime of the object.

@andoma
Copy link
Contributor

andoma commented Oct 5, 2015

Yeah, the threading infrastructure in SM was very complicated. Multiple native threads were allows to execute concurrently in the same runtime to some extent. I can't remember the details.

But running multiple threads was also buggy, sometimes it crashed more or less randomly and upgrading was not really an option for me as SM had grown to a big hairy beast.

This was why I gave up on SM and started to search for an alternative and luckily found Duktape.

@fatcerberus
Copy link
Contributor

@svaarala Concretely, what is preventing this from being feasible right now, in master? I assume it's more complicated than just the curr_thread pointer.

@svaarala
Copy link
Owner Author

svaarala commented Oct 5, 2015

I'd have to look up my old notes on that, but it's just a bunch of small book-keeping things here and there, basically anything to do with heap level state which doesn't get stored/restored automatically. There are also some assumptions in call handling which caused issues but those have mostly been resolved by earlier reworks.

@svaarala svaarala added this to the v2.0.0 milestone Aug 3, 2016
@svaarala svaarala added the api label Aug 3, 2016
@svaarala svaarala modified the milestones: v2.1.0, v2.0.0 Dec 20, 2016
@svaarala
Copy link
Owner Author

Moving the final cleanup issue, improving the suspend/resume structure representation, to 2.1.0 because it can be made in a compatible fashion.

@svaarala svaarala modified the milestones: v2.2.0, v2.1.0 Apr 8, 2017
@svaarala svaarala modified the milestones: v2.3.0, v2.2.0 Aug 23, 2017
@paolo-losi
Copy link

@svaarala how complex is this? would you accept a bounty?

@svaarala
Copy link
Owner Author

@paolo-losi Could you clarify what feature you mean? The main feature tracked by this issue has already been implemented in the form of duk_suspend() and duk_resume() API calls. The only remaining issue is internal header declaration trivia which is not visible to the application.

@paolo-losi
Copy link

@svaarala I am referring to supporting the usecase that you outlined in the main comment. Can you confirm that duk_suspend() / duk_resume() are enough for supporting that usecase? If yes, I guess only internal cleanup prevents the bug to be closed, right?

@svaarala
Copy link
Owner Author

Yes, the suspend/resume use case is functional, and the only checkbox preventing closing of this issue is the header cleanup.

@svaarala
Copy link
Owner Author

Moved the remaining cleanup issue to #1847 for clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants