Make iterators and generators thread-safe, but not necessarily MT

RESOLVED WORKSFORME

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED WORKSFORME
11 years ago
3 years ago

People

(Reporter: brendan, Assigned: gal)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
Any SpiderMonkey embedding that uses JS_THREADSAFE could implement an iterator with native code and, with the right mutual exclusion, make it both thread-safe and multi-threaded -- the latter meaning usable from several threads racing to call .next() at once.

But by default, the native iterator and generator types should not take on the cost imposed by MT accesses.  They need at most to be thread-safe (as in, you can't crash or subvert memory safety or other safety properties by abusing them from two threads running concurrently).

More on how to do this.  Not necessarily a 1.8.1 blocker but targeting at 1.8.1, to make sure there aren't problems we need to fix even in Firefox 2.

/be

Comment 1

11 years ago
A trivial way to fix this for now would be to say that calling next, send, throw and close is allowed only from the same thread that created the generator() instance and calling them from any other thread explicitly triggers a runtime exception. And E4X cursors can be fixed similarly. 

To implement this one would need to store the original thread in JSGenerator.

Comment 2

11 years ago
Created attachment 234589 [details] [diff] [review]
Implementation

Here is a trivial implementation of creator-thread-only idea build on top of the patch from bug 349320.
Assignee: brendan → igor.bukanov
Status: NEW → ASSIGNED
Attachment #234589 - Flags: review?(brendan)

Updated

11 years ago
Attachment #234589 - Attachment description: Impementation → Implementation
(Reporter)

Comment 3

11 years ago
Comment on attachment 234589 [details] [diff] [review]
Implementation

Neat, simple too.  Does the patch need updating to track the "Generator cleanup" bug's final patch?

>--- .pc/no_multithreading_generators.patch/js/src/js.msg	2006-08-19 03:02:33.000000000 +0200
>+++ js/src/js.msg	2006-08-19 17:00:19.000000000 +0200
>@@ -290,8 +290,9 @@ MSG_DEF(JSMSG_WRONG_CONSTRUCTOR,      20
> MSG_DEF(JSMSG_BAD_GENERATOR_RETURN,   208, 1, JSEXN_TYPEERR, "generator function {0} returns a value")
> MSG_DEF(JSMSG_BAD_ANON_GENERATOR_RETURN, 209, 0, JSEXN_TYPEERR, "anonymous generator function returns a value")
> MSG_DEF(JSMSG_NAME_AFTER_FOR_PAREN,   210, 0, JSEXN_SYNTAXERR, "missing name after for (")
> MSG_DEF(JSMSG_IN_AFTER_FOR_NAME,      211, 0, JSEXN_SYNTAXERR, "missing in after for")
> MSG_DEF(JSMSG_BAD_ITERATOR_RETURN,    212, 2, JSEXN_TYPEERR, "{0}.{1} returned a primitive value")
> MSG_DEF(JSMSG_KEYWORD_NOT_NS,         213, 0, JSEXN_SYNTAXERR, "keyword is used as namespace")
> MSG_DEF(JSMSG_BAD_GENERATOR_YIELD,    214, 1, JSEXN_TYPEERR, "yield from closing generator {0}")
> MSG_DEF(JSMSG_NESTING_GENERATOR,      215, 1, JSEXN_TYPEERR, "already executing generator {0}")
>+MSG_DEF(JSMSG_BAD_GENERATOR_THREAD,   216, 1, JSEXN_TYPEERR, "calling generator operation from a thread that did not create the instance of {0}")

How about shorter message that names the operator?

"calling {0} from a thread that did not create the generator {1}"

/be

Comment 4

11 years ago
(In reply to comment #3)
> (From update of attachment 234589 [details] [diff] [review] [edit])
> Neat, simple too.  Does the patch need updating to track the "Generator
> cleanup" bug's final patch?

Yes.

> How about shorter message that names the operator?
> 
> "calling {0} from a thread that did not create the generator {1}"

For future compatibility I think it is better to put in the updated patch just   
"calling {0} from a thread that do not own {1}".

I would like to extend the same thread-protection to cover xml cursors (straightforward) and iterators (messy since the iterator itself does not have space to store the thread), and reusing the message again would be handy. 
(Reporter)

Comment 5

11 years ago
(In reply to comment #4)
> (In reply to comment #3)
> > How about shorter message that names the operator?
> > 
> > "calling {0} from a thread that did not create the generator {1}"
> 
> For future compatibility I think it is better to put in the updated patch just  
> "calling {0} from a thread that do not own {1}".

Ok -- I guess this message would named be JSMSG_BAD_THREAD_ACCESS or some such instead.  That suggests further generalization, below.

> I would like to extend the same thread-protection to cover xml cursors
> (straightforward) and iterators (messy since the iterator itself does not have
> space to store the thread), and reusing the message again would be handy. 

When compiling JS_THREADSAFE, all native objects have OBJ_SCOPE(obj)->ownercx, which if null means the object has been accessed by (contexts used by) two or more threads in overlapping requests.

Perhaps we could use OBJ_SCOPE(obj)->ownercx and a SCOPE_SINGLE_THREAD flag to enforce ownercx invariance, throwing the JSMSG_BAD_THREAD_ACCESS on any attempt to call any object operation from a non-owning thread?

/be
(Reporter)

Comment 6

11 years ago
We would want to avoid conflacting context (ownercx) and thread (ownercx->thread) so as to allow accesses from the same thread using two different contexts.

/be

Comment 7

11 years ago
Created attachment 236058 [details] [diff] [review]
Implementation v2

The patch protects both iterator and generators against multi-threading access via checking for OBJ_SCOPE(obj)->ownercx. AFAICS the check can not be made generic with a special flag on 1.8.1 branch as there is no space left for yet another function flag.
Attachment #234589 - Attachment is obsolete: true
Attachment #236058 - Flags: review?(brendan)
Attachment #234589 - Flags: review?(brendan)
(Reporter)

Comment 8

11 years ago
Comment on attachment 236058 [details] [diff] [review]
Implementation v2

>Index: js.msg
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/js.msg,v
>retrieving revision 3.67
>diff -U7 -p -r3.67 js.msg
>--- js.msg	26 Aug 2006 20:24:45 -0000	3.67
>+++ js.msg	30 Aug 2006 09:28:02 -0000
>@@ -290,7 +290,8 @@ MSG_DEF(JSMSG_WRONG_CONSTRUCTOR,      20
> MSG_DEF(JSMSG_BAD_GENERATOR_RETURN,   208, 1, JSEXN_TYPEERR, "generator function {0} returns a value")
> MSG_DEF(JSMSG_BAD_ANON_GENERATOR_RETURN, 209, 0, JSEXN_TYPEERR, "anonymous generator function returns a value")
> MSG_DEF(JSMSG_NAME_AFTER_FOR_PAREN,   210, 0, JSEXN_SYNTAXERR, "missing name after for (")
> MSG_DEF(JSMSG_IN_AFTER_FOR_NAME,      211, 0, JSEXN_SYNTAXERR, "missing in after for")
> MSG_DEF(JSMSG_BAD_ITERATOR_RETURN,    212, 2, JSEXN_TYPEERR, "{0}.{1} returned a primitive value")
> MSG_DEF(JSMSG_KEYWORD_NOT_NS,         213, 0, JSEXN_SYNTAXERR, "keyword is used as namespace")
> MSG_DEF(JSMSG_BAD_GENERATOR_YIELD,    214, 1, JSEXN_TYPEERR, "yield from closing generator {0}")
>+MSG_DEF(JSMSG_ONLY_ONE_THREAD,        215, 2, JSEXN_TYPEERR, "calling {0} on object {1} that is accessed from multiple threads.")

Suggest "method {0} called on {1} from concurrently executing threads".

>Index: jsiter.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsiter.c,v
>retrieving revision 3.37
>diff -U7 -p -r3.37 jsiter.c
>--- jsiter.c	29 Aug 2006 20:37:49 -0000	3.37
>+++ jsiter.c	30 Aug 2006 09:28:02 -0000
>@@ -119,14 +119,52 @@ JSClass js_IteratorClass = {
>     JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,
>     JS_EnumerateStub, JS_ResolveStub,   JS_ConvertStub,   JS_FinalizeStub,
>     JSCLASS_NO_OPTIONAL_MEMBERS
> };
> 
> #if JS_HAS_GENERATORS
> 
>+#ifdef JS_THREADSAFE
>+# define CHECK_SINGLE_THREADED(cx, obj, argv, whenMultiThreded)               \

s/Threded/Threaded/

To make this generic, why not use a SCOPE_SINGLE_THREADED flag instead of a function flag?  The locus of single-threaded-ness is the object, not one or some of its methods.

/be

Comment 9

11 years ago
(In reply to comment #8)
> To make this generic, why not use a SCOPE_SINGLE_THREADED flag instead of a
> function flag?  The locus of single-threaded-ness is the object, not one or
> some of its methods.

But what about someOtherGeneratorInstance.next.call(openedGeneratorOnAnotherThread) ? 

If the flag belongs to the class, it means that for every function call it is necessary to access the class of this. Do we want such check?
(Reporter)

Comment 10

11 years ago
(In reply to comment #9)
> (In reply to comment #8)
> > To make this generic, why not use a SCOPE_SINGLE_THREADED flag instead of a
> > function flag?  The locus of single-threaded-ness is the object, not one or
> > some of its methods.
> 
> But what about
> someOtherGeneratorInstance.next.call(openedGeneratorOnAnotherThread) ? 

The object referenced by openedGeneratorOnAnotherThread will be no longer in use by a request active on that other thread, or else will be shared among threads with a thin lock instead of the ownercx flyweight lock.  In the first case, the access will be allowed on the current thread, and I think it should be.  In the second, per your patch's null test of ownercx_, access will be denied.

IOW, when requests take turns accessing an iterator object that somehow becamse accessible among threads, access is allowed.

But if a request should try to lock the object while it's in flyweight ownership use by another thread running in a request of its own, the object will be switched to thin lock ownership, and it could be subject to fine-grained thread-safe accesses among any threads running in racing requests.  In this case, access should be denied.

> If the flag belongs to the class, it means that for every function call it is
> necessary to access the class of this. Do we want such check?

We already have it: http://lxr.mozilla.org/mozilla/source/js/src/jsinterp.c#530

So JSCLASS_SINGLE_THREADED, not SCOPE_SINGLE_THREADED.

/be
(Reporter)

Comment 11

11 years ago
It's a matter of definition, but JSCLASS_SINGLE_REQUEST might be a better name.

/be

Comment 12

11 years ago
(In reply to comment #10)

> > But what about
> > someOtherGeneratorInstance.next.call(openedGeneratorOnAnotherThread) ? 
> 
> The object referenced by openedGeneratorOnAnotherThread will be no longer in
> use by a request active on that other thread, or else will be shared among
> threads with a thin lock instead of the ownercx flyweight lock.

...

> > If the flag belongs to the class, it means that for every function call it is
> > necessary to access the class of this. Do we want such check?
> 
> We already have it: http://lxr.mozilla.org/mozilla/source/js/src/jsinterp.c#530

Thanks, I missed that.

Just to get it right: since ComputeThis calls OBJ_GET_CLASS that calls ClaimScope to do the thread magic to claim the scope, it is necessary just to check that ownercx is not null for objects with JSCLASS_SINGLE_THREADED set? I.e. after OBJ_GET_CLASS either ownercx is null or ownercx == cx? 

Regards, Igor
(Reporter)

Comment 13

11 years ago
(In reply to comment #12)
> (In reply to comment #10)
> 
> > > But what about
> > > someOtherGeneratorInstance.next.call(openedGeneratorOnAnotherThread) ? 
> > 
> > The object referenced by openedGeneratorOnAnotherThread will be no longer in
> > use by a request active on that other thread, or else will be shared among
> > threads with a thin lock instead of the ownercx flyweight lock.

BTW, AOL made me patent the request-amortized flyweight object locking invention, but the MPL's patent clause grants everyone in the world right to use it in its SpiderMonkey implementation.

> > > If the flag belongs to the class, it means that for every function call it is
> > > necessary to access the class of this. Do we want such check?
> > 
> > We already have it: http://lxr.mozilla.org/mozilla/source/js/src/jsinterp.c#530
> 
> Thanks, I missed that.
> 
> Just to get it right: since ComputeThis calls OBJ_GET_CLASS that calls
> ClaimScope to do the thread magic to claim the scope, it is necessary just to
> check that ownercx is not null for objects with JSCLASS_SINGLE_THREADED set?
> I.e. after OBJ_GET_CLASS either ownercx is null or ownercx == cx?

Yes.

/be

Comment 14

11 years ago
Created attachment 236204 [details] [diff] [review]
Implementation v3

Here is universal JSCLASS_SINGLE_THREADED flag approach. The patch marks iterator, generator and XML classes with the new flag and enforces the restriction in js_ComputeThis. But to protect XML it is not enough. XML objects exposes their state through properties, so for them more has to be done.
Attachment #236058 - Attachment is obsolete: true
Attachment #236204 - Flags: review?(brendan)
Attachment #236058 - Flags: review?(brendan)

Comment 15

11 years ago
Morphing the title to better reflect a generic nature of the problem the latest patch tries to address.
Summary: Make iterators and generators thread-safe, but not necessarily MT → Make iterators, generators and XML objects thread-safe, but not necessarily MT
(Reporter)

Comment 16

11 years ago
I'll review this week.

/be
Blocks: 355044
(Reporter)

Comment 17

10 years ago
Igor, can you refresh this patch easily? If not I will do it. I should have reviewed it a while ago, obviously -- sorry about that!

/be

Updated

9 years ago
Blocks: 246441

Updated

9 years ago
Priority: -- → P5
Target Milestone: mozilla1.8.1 → ---
(Reporter)

Comment 18

9 years ago
Stealing, making a blocker for array thread safety.

/be
Assignee: igor → brendan
Blocks: 419537
Status: ASSIGNED → NEW
Flags: blocking1.9+
Priority: P5 → P1
Target Milestone: --- → mozilla1.9beta4

Updated

9 years ago
Attachment #236204 - Flags: review?(brendan)
Target Milestone: mozilla1.9beta4 → mozilla1.9
Quick poke here.  How are we doing on this, Brendan?  

Updated

9 years ago
Priority: P1 → P2
(Reporter)

Comment 20

9 years ago
I'm behind on this. We could try to get away with it and see how that goes. This bug blocks js1.8src release.

/be
Blocks: 428420

Comment 21

9 years ago
Does "get away with it" mean land Igor's "Implementation v3" patch?  or does that mean slip this beyond 1.9?
Brendan, think we should do this in a dot release?  This seems scary.
(Reporter)

Comment 23

9 years ago
Yeah, sorry -- comment 20 meant to say that.

/be
Status: NEW → ASSIGNED
Flags: blocking1.9+
(Reporter)

Updated

9 years ago
Target Milestone: mozilla1.9 → ---

Updated

9 years ago
No longer blocks: 428420

Updated

9 years ago
Blocks: 380236

Updated

9 years ago
No longer blocks: 380236

Updated

9 years ago
Blocks: 428420
No longer blocks: 428420
(Reporter)

Updated

7 years ago
Assignee: brendan → jorendorff
Releasing this bug -- I haven't been working on it.
Assignee: jorendorff → general
(Assignee)

Comment 25

7 years ago
I think I actually fixed this.
Assignee: general → gal
(Reporter)

Comment 26

7 years ago
Don't see how, but we could WONTFIX this, or make it depend on the MT proxies bug (bug 558866 IIRC).

/be
(Assignee)

Comment 27

7 years ago
Maybe I misunderstand the bug but now sharing an iterator no longer misbehaves any more than executing unsynchronized JS code outside an iterator. If you pull from an iterator from 2 threads, you get always valid js values, nothing crashes. Your program will behave weird and erratically, but thats really a high level application error. Am I missing something here?
(Assignee)

Comment 28

7 years ago
Brendan, what do we want to do with this bug?
(Reporter)

Comment 29

7 years ago
Native objects will be ST, so they become MT proxies if desirable/possible/easy, else throw. Case by case, we could make different arguments, but it would be fine to start by making these classes throw rather than "become". We need some code to do that, though, so there's a patch wanted here.

/be
No longer blocks: 246441
Summary: Make iterators, generators and XML objects thread-safe, but not necessarily MT → Make iterators and generators thread-safe, but not necessarily MT

Comment 30

3 years ago
Solved by single-threaded runtime.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.