I'm not entirely convinced that supporting the specific case where one
thread is in an Unlocker scope and then allows another thread to enter the
same isolate is worth it -- I fear there might be many assumptions that
might easily get broken in that scenario. (If you want fool-proofing, maybe
there should be a DCHECK to guard against this.) What I meant in the other
thread is that the existing PerIsolateThreadData implementation supports
having a pool of threads and a pool of isolates, where any thread can enter
any free isolate, do some work there, and leave it. In many cases you'd
probably just have fixed Thread/Isolate pairs though.
Also, a word of caution: changing the API causes a lot of churn for many
embedders. When there's a strong reason to change it, we shouldn't let that
concern stop progress; but personal opinions ("...seems pretty sketchy")
are likely not enough justification to force many people to do extra work.
That's one reason why I suggested a separate API call: existing embedders
that follow one of the currently supported usage patterns can simply ignore
it.
On Thu, Oct 22, 2015 at 8:55 AM, Jochen Eisinger <[email protected]>
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]> 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]
>> 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.
>>
> --
> --
> 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.
>
--
--
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.
