561657 - Replacing js_CountThreadRequests with an explicit counter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently to count the number of requests running on the current thread the code enumerates all contexts to find all contexts where the request count is not zero. For the GC purposes this is fine even if there are hundreds of JSContext per thread (like in a browser session with 100 tabs/windows) as the GC enumerates all contexts in any case. 

But for the bug 516832 it would be necessary to check for the request counter for the current thread in JS_SuspendRequest to find the call that really suspends the request. So to make sure that JS_SuspendRequest would not slow down and for code simplicity I suggest to replace js_CountThreadRequests with a counter field in JSThread that is updated as necessary JS_BegingRequest/JS_EndRequest.

Comment 1

[Igor Bukanov]

Attachment: v1

I wish it would be possible to address the bug 477999 without a surgery of xpconnect interfaces to avoid all the complexity of managing multiple contexts in requests per thread. But for now at least we can avoid the counting.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 441396 [details] [diff] [review]
v1

I think it would be cool to have the invariant

  rt->requestCount = sum of thread->contextsInRequests

Then thread->contextsInRequests would obviously need to be adjusted exactly in those places where rt->requestCount is adjusted.

Alternatively, put a little more detail into the comment in JSThread explaining precisely what the new field counts.

(I ask for this because we now have quite a few request-counting fields, and they all count slightly different things.)

r=me with either change.

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> (From update of attachment 441396 [details] [diff] [review])
> I think it would be cool to have the invariant
> 
>   rt->requestCount = sum of thread->contextsInRequests
> 
> Then thread->contextsInRequests would obviously need to be adjusted exactly in
> those places where rt->requestCount is adjusted.

Currently the invariant is violated when the GC waits for other threads to finish their requests and when the GC thread requests are discounted. This way rt->requestCount == 0 can be used as a condition to send a signal for the GC to wake it up from the wait method. We can patch that so the condition woule be
 rt->requestCount == (rt->gcThread ? rt->gcThread->contextsInRequests : 0). But I am not sure that it is worthwhile especially since I would like to move the request counting to JSThread so JS_SuspendRequest will reliably suspend all the request, see bug 477999.

So I will add comments

Comment 4

[Igor Bukanov]

Attachment: v2

Here is a patch with better comments. I will land it after resolving the bug 561364 to avoid an extra merge efforts if the patch in that bug would be OK.

Comment 5

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/7257ebf9d582

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7257ebf9d582