Jochen, OK, I've built the code to allow arbitrary entry and exit of multiple isolates by multiple threads. The main "trick" was to create a per-thread object (PerThreadData) that could be used as an anchor for tracking thread entries and exits. The project eliminates the need for the ThreadDataTable mutex (because it eliminated the ThreadDatatable) and automatically cleans up per-isolate-thread data on the last exit and unlock of an isolate by a thread. It also eliminates the need for an EntryStackItem if a thread only uses a single isolate (so eliminating a new and presumably malloc). The project was relatively simple though maybe I violated protocol by moving some stuff from isolate to v8threads where it made a lot more sense. I could have done a lot more -- ThreadState seems kinda redundant in that its data could easily be moved to PerIsolateThreadData (I think) and the thread's current isolate pointer could have been placed into PerThreadData instead of using a separate base::Thread::LocalStorageKey. But, I guess I had to draw a line somewhere.
I also added some code to test-api.cc to test a variety of thread/isolate entry/exit combinations, including the issue I originally reported as the Isolate entry stack bug. This test code was trickier than the base project code but it's working now and kinda fun -- it's easy to add thread/isolate entry/exit scenarios. I felt bad putting this int test-api.cc as this is kind of a beast now but, since my changes are largely intended to improve embedder API behavior in a multi-isolate/thread environment it seemed like a logical place. I'd be happy to move it to test-threads though that seems to be more geared toward testing thread internals. Or maybe test-lockers though that did seem quite right either. Note that there are no embedder API changes in this project nor any changes in v8 behavior other than fixing thread stacking and thread cleanup. In any case, I'm not sure where to go now. I've tested all my changes locally and all the tests pass though I'll admit I only did an X64 (release and debug) test as I imagine anything else will take forever on my x86 linux machine. So should I git cl upload my project? Thanks! On Wednesday, October 21, 2015 at 11:55:52 PM UTC-7, Jochen Eisinger wrote: > > Hey, > > thanks for the detailed analysis. Currently, we conceptually support two > modes: either you use one isolate per thread, or you use one isolate and > share it with many different threads. > > It's of course possible to use multiple isolates the way you described it, > and if you want to contribute code to make this possible, that's highly > appreciated. > > As you point out, we work towards having the embedder always provide the > isolate to use, so we would no longer have to store any data in tls, but > that's still far out in the future. > > best > -jochen > > On Wed, Oct 21, 2015 at 7:27 PM Alex Kodat <[email protected] > <javascript:>> wrote: > >> Since my last e-mail generated so much excitement I guess I'll toss out a >> proposal for how I would fix this and, while the heart is on the table, >> deal with the thread cleanup issue that led me down this path. >> >> First, I'll note that I suspect that the lack of interest in the bug that >> I described (and might well be wrong about) is that there is a hope that >> the entry stack is going away soon anyway. Since its only purpose seems to >> be to set the result of Isolate::GetCurrent which seems to be on the road >> to deprecation maybe no one cares? However, if the entry stack will be >> around for at least another year or two, I think it should be fixed. If >> not, then my proposal below might still make sense but is probably less >> compelling. >> >> Whether or not Isolate::GetCurrent is going away, I'll opine that the >> Isolate::Enter and Isolate::Exit should be deprecated and people should >> just use Isolate::Scope (which ultimately will be deprecated or at least >> no-oped, too?) as the DIY Enter/Exit matching seems pretty sketchy and not >> particularly useful. >> >> Leaving that aside, I would propose a PerThreadData anchor block that >> each thread has at most one of and is accessed >> via per_isolate_thread_data_key_ (perhaps renamed to per_thread_data_key_). >> This block could contain an anchor for the PerIsolateThreadData for the >> owning thread and, if the task entry stack is not going way, the >> EntryStackItem stack and even the current isolate; again, likely to go away >> but, for now, obviating the need for the isolate_key_ thread key. >> >> In any case, a PerThreadData anchor block for the PerIsolateThreadData >> would eliminate the need for a mutex to find, add, or remove >> PerIsolateThreadData and would make safe/automatic cleanup of this data >> much easier. In my original post on thread data cleanup Jakob had stongly >> opined that I just just propose a separate call >> DiscardThreadSpecificMetadata() to allow embedders to clean up after >> themselves in threads because he felt doing it automatically was too risky >> which I thought was fair enough but, if folks feel the entry stack issues >> should be fixed too, then the automatic PerIsolateThreadData cleanup seems >> a pretty minor addition to such a project. It seems it would be preferable >> to do things automatically than have yet another embedder method. >> >> As far as testing what I'm proposing goes, I think the hacked hello-world >> demonstration of the entry stack bug could be converted fairly easily into >> a relatively general purpose thread test harness (but maybe there already >> is one?). Its use of a semaphore array could be combined with thread >> specific arrays that map out each thread's plan of attack. The semaphore's >> ensure deterministic ordering of each thread's isolate >> entry/exit/locking/unlocking. But probably there's already something out >> there for testing lazy/eager archiving/restoring so I'd add appropriate >> tests using that, if possible. >> >> On Tuesday, October 20, 2015 at 9:22:42 AM UTC-7, Alex Kodat wrote: >>> >>> Jakob had suggested that I post a proposal to v8-users related to a thread >>> cleanup issue >>> <https://groups.google.com/forum/#!topic/v8-users/iZ1gz1KThgs> which I >>> plan on doing, but his comments about threads going in and out of isolates >>> got me curious about thread entry/exit mechanics which led me to puzzlement >>> about the use of Isolate::entry_stack_. Specifically, it seemed to make no >>> sense at all to me for this to be in Isolate. The entry stack is a per >>> thread thing and no one other than the thread that owns a stack entry needs >>> to look at it so why is it in Isolate? It wouldn't matter if this were >>> harmless but it seems that Isolate::Enter happily sets a pointer to another >>> thread's EntryStackItem if one happened to be in entry_stack_ for the new >>> Isolate and Isolate:Exit then happily sets it back to whatever it was when >>> it entered the isolate, regardless of what that thread or other threads >>> that might have entered this Isolate are doing. Having one thread messing >>> with another's stack entries seems clearly wrong since obviously one point >>> of threads is to allow independent stacks for the threads. >>> >>> I've attached a hacked version of hello-world (sorry, it probably only >>> runs on linux and maybe some unixes) that demonstrates a bug check that >>> goes off in Isolate::Exit as a result of this problem. The basic sequence >>> is that there are two threads and two isolates: >>> >>> 1. Thread 0 enters isolate 1 - Sets entry_stack_ for that isolate to >>> say A0 >>> 2. Thread 1 enters isolate 1 - Sets entry_stack_ for that isolate >>> to say A1 - A1->previous_item_ == A0. >>> 3. Thread 0 enters isolate 2 - Sets entry_stack_ for that isolate to >>> say B0 >>> 4. Thread 1 enters isolate 2 - Sets entry_stack_ for that isolate to >>> say B1 - B1->previous_item_ == B0 >>> 5. Thread 0 exits isolate 2 - It finds entry_stack_ pointing to B1 >>> and then finds B1->previous_thread_data pointing to thread 1's data and >>> the >>> bug check goes off. End of story. >>> >>> The bug check that goes off is in Isolate::Exit >>> >>> DCHECK(entry_stack_->previous_thread_data == NULL || >>> entry_stack_->previous_thread_data->thread_id().Equals( >>> ThreadId::Current())); >>> >>> Obviously this only goes off for a debug build of the attached code. I >>> imagine one could produce a spectacular mess with a release version if one >>> used more isolates and threads but I think the debug build is sufficient to >>> prove the point. >>> >>> While this could be fixed by thread data archive and restore, archiving >>> and restoring entry_stack_ this would seem silly to me as again, there >>> seems nothing particularly useful about having entry_stack_ in Isolate. It >>> would seem that the best approach would be to somehow combine >>> EntryStackItem with PerIsolateThreadData or at least set the latter to >>> point at the former, but since I might be wrong about all this and would >>> need to do quite a bit more research even if I weren't, I won't float a >>> proposal here. >>> >>> Maybe I should have posted this to v8-users? But this seems kinda >>> internalsy to me so v8-dev seemed right. >>> >>> Thanks >>> >> -- >> -- >> v8-dev mailing list >> [email protected] <javascript:> >> http://groups.google.com/group/v8-dev >> --- >> You received this message because you are subscribed to the Google Groups >> "v8-dev" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected] <javascript:>. >> For more options, visit https://groups.google.com/d/optout. >> > -- -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
