477187 - Eliminate operationCount

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andreas Gal :gal]

Attachment: patch, incomplete, untested, for discussion only

This patch removes the operationCount. Both operationCallback and branchCallback only trigger when js_TriggerOperationCallback() is invoked, which is allowed from any thread. A counter in JSContext indicates to leave any running native code and the interpreter checks the operationCallbackRequest flag at every loop edge.

When starting a GC, all contexts are forced back into the interpreter. I am not exactly clear how the current callback yeilds the pending request so we can proceed with the GC. This part needs a bit more investigating. 

I am posting a very very preliminary incomplete untested sketch of a patch.

Comment 1

[Andreas Gal :gal]

Attachment: v2

I attached a new version of the patch and tries to solve the GC problem.

When we enter js_GC we trigger the execution of the operationCallback for each context (except the current one). This allows the embedding to yield the current request which again will unblock the running thread that is trying to GC.

If the embedding does not yield the request during the callback, we are hosed (since there will not be another time-based callback). I am not sure whether this is a problem or not. Opinions?

Comment 2

[Brendan Eich [:brendan]]

Embeddings have to yield, suspend/resume, etc., but the API should distinguish those callbacks for which it is mandatory to yield from all others. We can evolve the operation callback as needed, it's new.

/be

Comment 3

[Andreas Gal :gal]

Attachment: v3

Attached is a working version of the watchdog patch. I have wired up the shell to spawn a watchdog thread when compiled with JS_THREADED. This used to happen on demand in scatter. I don't see a point in that. Its now started at startup and runs the entire time. The watchdog thread triggers every second. Its up to the embedding to chose this granularity. 

At every watchdog interval, all contexts are requested to invoke their respect callbacks. This is done by setting operationCallbackRequest in JSContext to 1. Repeated requests are ignored.

If a context is running in jit code, we leave the trace at the next loop header. Once back in the interpreter, the next loop edge instruction invokes the operation callback.

Once the operation callback executes, the first thing it does is to yield. This is important because js_GC signals all operation callbacks right after the main thread entered the GC phase. Operation callbacks _must_ yield in _every_ callback, or the world is going to end. I will add an assert or a work around to catch this in the next patch.

If a context is not currently executing, it will not get the signal and not call its callback. The callback will trigger whenever the context is scheduled again.

The shell uses a fixed 1 second watchdog interval, so a 3 second timeout can take anywhere from 3 to 4 seconds. I think this is more than sufficient for our purposes. For mobile platforms we probably want to further increase the interval to save power.

New commands in the shell:

timeout(x) sets the timeout from 1..N seconds (N<=3600). 
elapsed() returns the microseconds.

Comment 4

[Andreas Gal :gal]

Note: this patch also eliminates operation counting from the interpreter, which should yield a small speedup. I will try to measure this in a bit.

Comment 5

[Andreas Gal :gal]

This patch yields a 10ms speedup in the single-threaded shell.

Comment 6

[Andreas Gal :gal]

Attachment: v4

This patch asserts that the operation callback always yields.

Comment 7

[Andreas Gal :gal]

Attachment: v4, this time for real (qdiff, not diff, its getting late ...)

Comment 8

[Andreas Gal :gal]

Example for the timeout in action:

./js -j -e 'timeout(3); while(1);'
recorder: started(1), aborted(0), completed(1), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(0), breaks(0), returns(0), unstableInnerCalls(0)
monitor: triggered(4), exits(0), type mismatch(0), global mismatch(0)

Note that we trigger the tree 4 times here, since every second the watchdog timer forces a timeout exit.

Comment 9

[Igor Bukanov]

Comment on attachment 360882 [details] [diff] [review]
v4, this time for real (qdiff, not diff, its getting late ...)

Quick observations:

1. The patch must keep ant least compile-time  option to use operation counting. Embeddings use it to schedule GC so forcing all of them to implement a separated thread is wrong and badly breaks the existing APIs. 

2. The patch has a race in InvokeOperationCallback: when the GC sets operationCallbackRequest, another thread can be at that moment in js_InvokeOperationCallback setting operationCallbackRequest to 0. While fixing this (I think taking the GC lock is the simplest thing to do) I suggest not to delegate to embeddings the responsibility of yielding under the GC but rather make the current thread to wait for the GC to complete.

Comment 10

[Andreas Gal :gal]

I am not strongly opposed to 1), however Brendan suggested the api is new enough to evolve it. Either way, I suggest we factor this out into its own bug "introduce compatibility layer, emulating previous operation counting approach". This sub-bug is not a b3 blocker in my opinion so we should focus at what we can and need to solve within the next 48h or so.

As for 2, I am open to yielding directly in js_InvokeOperationCallback instead of delegating this to the embedding if there is consensus that this is safe. I am attaching a new patch that attempts to fix the race condition. Let me know what you think.

Comment 11

[Andreas Gal :gal]

Attachment: v5

Comment 12

[Andreas Gal :gal]

Attachment: v6, fix the context iterator in the GC path

I used the following test to confirm that the GC correctly preempts other threads:

scatter([function() { print("1"); while (1); }, function() { print("2"); while (1); }, function () { print("3"); gc(); print("done");}]);

This is potentially racy and not suitable for automated testing, but it works on macosx at least. Note that the 2 threads run forever, except for the GC preemption.

Comment 13

[Andreas Gal :gal]

From a discussion with igor:

On anything but x86 we will need to deal with lack of cache coherence. The writes to operationCountRequest must flush into memory, and the read in the loop but also in the interpreter must carry a read barrier (I am looking at you sparc!). I don't see good support for this in NSPR so we might have to hack the assembly by hand.

Comment 14

[Igor Bukanov]

Currently even without the patch the jitted code would not contain the operation counter update checks when embedding does not set set operation limit on it. This is already true for js shell. With the watchdog patch for the browser, bug 453157, that would be the case for the browser as well.

Thus I suggest to split this patch into 3 parts:

1. Make sure that, when the embedding does not want the operation counting and relies exclusively on js_TriggerOperationCallback, the operation callback is called with 100% reliability. Currently, due to the problem outlined in the comment 13, this is not the case on all platforms we targets. Note also that this problem bites all users of JS_CHECK_OPERATION_LIMIT macro and not only the jitted code or the interpreter loop.

2. With that in place make the GC to auto-suspend the current request. This can go to the bug 477179 together with workers/js shell update to rely on this future.

3. Make the operation counting a compile-time option disabled in Firefox to speedup non-jited case.

Comment 15

[Andreas Gal :gal]

Ok, little update on the cache coherency discussions. Here is what we want:

One processor reads a memory location in a loop. Another processor wants to modify that location and cause the original processor to leave the loop. We have no strict ordering constraints. We only want the original processor to see the effect of the memory write "some time later".

As far as I can tell, this is fine on x86. The updating processor will write to main memory automatically, and the reading processor's cache controller will snoop that write and evict its own cached value and the resulting miss will re-read the value and the loop will be left.

As far as I can tell this is also true for sparc. We might need a membar instruction in the writer to guarantee that the value makes it all the way into main memory, but the reading processor will pick up on the write on its own so the reader loop does not have to poll main memory, which would be ridiculously slow. CC'ing Leon to confirm this.

I also briefly looked into PowerPC, and starting with 7400 this seems to be true there as well.

Comment 16

[Andreas Gal :gal]

CCing Moh as well to double check my claims above.

Comment 17

[Andreas Gal :gal]

Attachment: v7, automatically yield at every callback (embedding doesn't have to any more), and use AtomicSet to write to the request flag

As suggested before I would like to worry about compatibility in a separate bug. The branch/operation callbacks are broken enough by design that incompatible changes seem appropriate. I will defer to Brendan to decide what degree of API compatibility we want to maintain.

I assume Igor is mostly worried about single-threaded embeddings that cannot run a parallel watchdog thread. Even in such environments I am against re-introducing the old operation counting/weights mechanism (even as compile-time option). The mystical operation weights are entirely meaningless and deeply compiler and architecture dependent. Even in a single-threaded environment there is most likely a way to raise an alarm at regular intervals and its acceptable to use js_TriggerOperationCallback from interrupts since all it does is set a flag.

Comment 18

[Igor Bukanov]

I was wrong in the comment 9. That race is harmless - at that point we are inside the callback and will eventually take the GC lock when yielding. Thus there is no need to check if another call for JS_TriggerOperationCallback was made at the end of the loop.

Comment 19

[Igor Bukanov]

(In reply to comment #17)
> I assume Igor is mostly worried about single-threaded embeddings that cannot
> run a parallel watchdog thread. Even in such environments I am against
> re-introducing the old operation counting/weights mechanism (even as
> compile-time option).

I am worried about any embedding that uses the branch or operation callback that will be broken by this change and forced spending time on adding rather non-trivial code. Yes, the operation callback exists only from 1.9.0, but the branch callback is not. Thus breaking a support for it is like breaking any other well established API. 

Thus I suggest not to mix the items 1-2-3 in a single patch and focus on the real issue of auto yield (probably moving that patch of current patch into the bug 477179) and making sure that the issues from the comment 15 are addressed leaving the api breakage for another bug.

Comment 20

[Igor Bukanov]

(In reply to comment #17)
> The mystical operation weights are entirely meaningless

They are not mystical and comes from the compatibility requirements to make older code that is using the branch callback to work more-or-less in the same  way when the engine internally has started to use the operation counting. The problem was to ensure that the branch callback is called with approximately the same frequency as before while allowing for accounting of other potentially long operations.

Comment 21

[Andreas Gal :gal]

Yeah, agreed. I meant with meaningless mostly-meaningless-in-the-terms-of-real-time-taken, which is what the browser seems to really care about. 

I want to avoid using operation count weights to emulate real time if we can get real time easier and cheaper. 

However, as you pointed out some embeddings might not be able to use real time and a watchdog thread, so we might have to reintroduce the weights and then ignore them in multi-threaded environments. We can also try to re-calibrate the operation weights based on real time, but that will likely only be a rough approximation of the previous behavior.

Comment 22

[David Mandelin [:dmandelin]]

We currently have an implementation of the branch callback API that works with tracing, right? And does nothing unless the user asks for it through JSAPI? It seems we can keep that for compatibility while going ahead with the new mechanism. 

Presumably if JS runs faster, that will affect the frequency of the branch callback. So it seems we must tell embedders that they are not allowed to assume anything about the timing of branch callbacks, which would give us a bit more freedom.

Comment 23

[Andreas Gal :gal]

I am not sure what you mean. How would a trace trigger a branch callback? We don't update the counter any more as of this patch. With this patch the embedding has to signal trace exit via a separate thread. Do you want to re-insert the counting on trace if a branch callback is set?

Comment 24

[Igor Bukanov]

(In reply to comment #22)
> We currently have an implementation of the branch callback API that works with
> tracing, right? 

Yes, the current implementation of the *operation callback* functionality works both with the jited code and unjited code.

> And does nothing unless the user asks for it through JSAPI? 

Sort of. The interpreter would still do useless checks and updates of the operation counter and the jited code would still contain the checks for the counter. But the jited code would not have instructions to update the counter, the biggest culprit to the performance impact.  

> Presumably if JS runs faster, that will affect the frequency of the branch
> callback. So it seems we must tell embedders that they are not allowed to
> assume anything about the timing of branch callbacks, which would give us a bit
> more freedom.

Well, that has being the message starting from 1.9.0, when the operation counting replaced older branch callback. While 1.9.0 kept full API compatibility with older embedding using the branch callbacks, the newer code could not guarantee that the branch callback would be called exactly once per branch in the script.

Comment 25

[David Mandelin [:dmandelin]]

General note: I think we need to be very clear in the comments/docs about the semantics/guarantees of this API. It looks to me like any given attempt to trigger the callback is not guaranteed to do anything: Imagine thread 1 tries to trigger a callback to f() and this sequence occurs:

      thread 0 (js)             thread 1                thread 2
 1                                                      write flag
 2                              write flag
 3    clear flag
 4    call callback

Only one callback occurs. Given that the 'again' mechanism is not reliable, do we need it? Getting rid of it, and simply setting the flag to 0 after calling the callback seems simpler, and also reduces the chance of livelock (e.g., if the callback continually tries to activate another callback). Besides, for our purposes it doesn't seem necessary to try to make the call count match up--we just need the callback to run at least once sometime around when it was asked for.

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> Only one callback occurs. Given that the 'again' mechanism is not reliable, do
> we need it?

That was done in the initial patch but then the "again" part was introduced due to wrong reasoning by me.

Comment 27

[Andreas Gal :gal]

Yeah, agreed, as igor pointed out we take the GC lock inside the yield (and now we guarantee there is a yield). I will switch back to the old scheme without the again and the flag being reset after the callback.

Comment 28

[Andreas Gal :gal]

Attachment: v8, reset operationCountRequest to 0 after the callback as per discussion with dmandelin+igor

Comment 29

[Igor Bukanov]

(In reply to comment #27)
> I will switch back to the old scheme without
> the again and the flag being reset after the callback.

The flag should be reset *before* calling the callback or doing yield.

Comment 30

[Andreas Gal :gal]

Right, otherwise we might miss a count if another request comes in after the yield. My bad. Little flag. So much trouble ;)

Comment 31

[Andreas Gal :gal]

Attachment: v9, must reset the flag before yield otherwise we might miss a count

Comment 32

[Igor Bukanov]

To make the scatter implementation for js shell work without frequent yields it is not enough to trigger the callback when the GC is about to wait for other requests to yield. The code must also do that in ClaimTitle from jslock.cpp to deal with objects shared across threads.

In theory this should not be an issue for thread workers as by design they should not share anything. But I am not sure that the current implementation avoids that for internal, not visible to scripts, JS objects.

But then again,why not to do that under the bug 477179?

Comment 33

[Andreas Gal :gal]

Attachment: v10, remove Elapsed() function (moved into the elapsed time patch)

Comment 34

[Andreas Gal :gal]

Attachment: v11, hide js_Invoke and introduce JS_Invoke that properly wraps the elapsed time handling

Comment 35

[Andreas Gal :gal]

Attachment: refreshed v10 as 12, v11 is bogus

Comment 36

[Andreas Gal :gal]

Attachment: v13, add RequestContextIterator and wrap interp again instead of api

Comment 38

[Andreas Gal :gal]

Attachment: v14

Comment 39

[Andreas Gal :gal]

Attachment: v15

Don't re-enter JIT until callback has occured.

Comment 40

[Andreas Gal :gal]

Attachment: v15, for real

Comment 41

[Andreas Gal :gal]

Important: the callback handler must disable the callback if it wants to re-enter the engine on the same context (or preferably any context). The current dom code didn't do that.

Comment 42

[Andreas Gal :gal]

We were able to successfully bring up a browser with this patch and the slow script dialog comes up. jst will post the browser-side patches.

Comment 43

[Andreas Gal :gal]

Attachment: v16

As per discussion with Brendan, this patch removes the old branch callback API calls altogether and incompatibly changes the operation callback API as follows:

JS_SetOperationCallback(cx, cb)
JS_GetOperationCallback(cx)
JS_TriggerOperationCallback(cx)

The higher level timeout mechanism is handled by the embedding. It can use the new JS_GetElapsedTime(cx) and JS_ResetElapsedTime(cx) API for this. Time elapses as scripts are interpreted or executed by the JIT. API calls into native code directly do not count towards this budget.

Threadsafe embeddings use a thread to trigger the callbacks. I added igor's js_RequestContextIterator function so embeddings can walk all contexts with currently pending requests and wake them up.

Single-threaded embeddings can use the following last-resort API to emulate a watchdog thread:

JS_SetHeartbeatCallback(rt, cb)
JS_GetHeartbeatCallback(rt)

Heartbeats occur every consecutive 4096 backedges in the same interpreter loop. Every re-entry into the interpreter loop resets the counter. The JIT does not trigger the heartbeat callback. Embeddings that want to use this have to disable the JIT.

Once the heartbeat callback fires the embedding uses the same operation callback mechanism to actually wake up contexts as multi-threaded embeddings.

Comment 44

[Igor Bukanov]

(In reply to comment #43)
> Created an attachment (id=361020) [details]
> v16

The patch still does not address the comment 32 about the waits due to object sharing.


> The JIT does not
> trigger the heartbeat callback. Embeddings that want to use this have to
> disable the JIT.

Why is this necessary? The tracer can add the heartbeat updates if they are requested. Currently the jited code is much more faster with !JS_THREADSAFE so a single threaded embedding should not be requested to activate JS_THREADSAFE just to get operation callback calls.

Comment 45

[Andreas Gal :gal]

I am currently fixing the 2nd comment (make JIT work with the heartbeat). Testing the patch atm.

The first I will have to look into a bit. I will try to get hold off you on IRC.

Comment 46

[Andreas Gal :gal]

Attachment: v17

This patch now also triggers the heartbeat from JIT code in single-threaded builds. On my machine I get about 880 heartbeats in an empty loop from the JIT. A lot less from the interpreter (50 or so per second). The heartbeat trigger code is only emitted when the heartbeat was set at recording time.

The single-threaded shell now only installs the heartbeat if -t was specified. The multi-threaded shell always installs the heartbeat since it also uses the callback to synchronize the GC (even if no timeout is desired).

This patch does not address the ClaimTitle issue yet.

Comment 47

[Andreas Gal :gal]

Attachment: v18

As per igor's guidance, also notify other contexts with pending requests before sleeping in ClaimTitle.

Comment 48

[Igor Bukanov]

Observation regarding putting time-monitoring code to the engine:

1. This is useless for DOM workers since they newer timed-out but rather canceled from the main thread. Thus updating time for contexts that run them is literally waste of time.

2. This is useless for a shell if it wants to implement universal timeout code that works, for example, across evalcx or scatter function bounding the total script execution time including time for all threads. Here the time the code spends in a aprticular context is irrelevant.

3. It is not very useful for the browser. With the removal of the need for explicit yield and doing the monitoring for the GC pressure from the watchdog thread (I need to file a bug aboit it) the timing in the engine could not be used to show the slow script warning dialog unless the engine emulates precisely what the code is doing in dom/src/base/nsJSEnvironment.cpp.

Comment 49

[Andreas Gal :gal]

Attachment: v19

v19: Igor pointed out that PRMJ_Now() is slow on Windows, so I am using now PR_IntervalNow() instead, which is supposed to be fast and lock-free. The documentation guarantees 6 hours till overflow. In addition we re-base the start time in every JS_YieldRequest(), which is now called automatically and frequently, making an overflow very unlikely. In addition to that, we guard against overflows by only allowing time to proceed forward, so worst case we miss a few ticks.

Whether there should be time handling in the engine at all is a different question. The timing mechanism and the per-context binding was requested by jst and bent. I have no strong feelings about it either way. If jst and bent agree that it is unnecessary, I am more than happy to drop it. jst? bent?

Comment 50

[Andreas Gal :gal]

Attachment: v20

Lock the GC every time the watchdog wakes up, not around the watchdog thread entry/exit.

Comment 51

[Andreas Gal :gal]

Comment on attachment 361038 [details] [diff] [review]
v20

I got the locking wrong in v20. Back to v19.

Comment 52

[Andreas Gal :gal]

Attachment: v21, replacing v19 (v20 was bogus)

Rename js_TriggerOperationCallbacksBlaBlaDontWriteABookInFunctionNames to js_NudgeOtherContexts(cx).

Comment 53

[Johnny Stenback (:jst)]

Attachment: First stab at watchdog thread for the browser.

This is based on Igor's patch in bug 453157, but is significantly simplified since there's no dealing with different watchdog intervals on different contexts. The browser starts and seems to run fine. The DOM code's operation callback is called and the slow script dialog works. This still doesn't use Andreas' time accounting code in the JS engine, but that should be easy to move to. This touches the DOM worker code to make it build, but I don't know what that code would actually do if it was hit yet :) Ben, wanna look at that part here?

Comment 54

[Igor Bukanov]

(In reply to comment #52)
> Created an attachment (id=361040) [details]
> v21, replacing v19 (v20 was bogus)

In the ClaimTitle there is no need to nudge all other contexts. It is sufficient to yield only the contexts that runs on the thread that currently owns the object. Unfortunately it is not possible to simply iterate over ownercx->thread->contextList as JSThread.contextList is manipulated outside the GC locks. One really has to go over JSRuntime.contextList and just check if the thread is ownercx->thread.



> 
> Rename js_TriggerOperationCallbacksBlaBlaDontWriteABookInFunctionNames to
> js_NudgeOtherContexts(cx).

Comment 55

[Igor Bukanov]

(In reply to comment #53)
> Created an attachment (id=361042) [details]
> First stab at watchdog thread for the browser.
> 
> This is based on Igor's patch in bug 453157, but is significantly simplified
> since there's no dealing with different watchdog intervals on different
> contexts.

HM, but I do not see why it is true. The DOM workers do not do periodic MaybeGC checks. With auto-yield under the GC the only thing that needs *periodic* callback calls there is the monitoring of the cancellation flag. So we have 2 very different periods in nature, the maybe gc checks on the main thread and cancellation checks for workers. I do not see how they can be the same. For example, is it acceptable to the worker to run for up-to 0.1s after canceling it?

Moreover, if, when setting the cancel flag, the DOM workers would also call JS_TriggerOperationCallback, any need for periodic callback calls would be removed. Alternatively, if that is too complicated to do under the current patch (Ben, can you comment on that?), the workers can use that heartbit for now. But then the watchdog thread would only need to monitor the main thread and, perhaps, the GC pressure (see bug  477367). Which suggests that the proper place for the watchdog is dom/src/base/nsJSEnvironment.

Comment 56

[Johnny Stenback (:jst)]

(In reply to comment #55)
> (In reply to comment #53)
> > Created an attachment (id=361042) [details] [details]
> > First stab at watchdog thread for the browser.
> > 
> > This is based on Igor's patch in bug 453157, but is significantly simplified
> > since there's no dealing with different watchdog intervals on different
> > contexts.
> 
> HM, but I do not see why it is true. The DOM workers do not do periodic MaybeGC
> checks. With auto-yield under the GC the only thing that needs *periodic*
> callback calls there is the monitoring of the cancellation flag. So we have 2
> very different periods in nature, the maybe gc checks on the main thread and
> cancellation checks for workers. I do not see how they can be the same. For
> example, is it acceptable to the worker to run for up-to 0.1s after canceling
> it?

It's fine for workers to keep executing JS for a while after they've been canceled, as long as they're not able to post messages or load scripts etc, which I believe is already blocked. What will have to happen, and bent is on this already, is that when we leave a page that's using workers, we'll go through the workers and flag all of them as canceled etc, and also for any of those that are currently running JS, we'll call JS_TriggerOperationCallback() so that they'll fall of trace if they're on trace, and eventually end up in their branch callback where they'll get canceled (or paused), as you mentioned here as well. There's no real need for workers to get a periodic call to their branch callbacks any longer, as long as they're guaranteed to get one shortly after they've been paused or canceled.

> Moreover, if, when setting the cancel flag, the DOM workers would also call
> JS_TriggerOperationCallback, any need for periodic callback calls would be
> removed. Alternatively, if that is too complicated to do under the current
> patch (Ben, can you comment on that?), the workers can use that heartbit for
> now. But then the watchdog thread would only need to monitor the main thread
> and, perhaps, the GC pressure (see bug  477367). Which suggests that the proper
> place for the watchdog is dom/src/base/nsJSEnvironment.

It also needs to ensure that any other contexts that are not DOM contexts and not DOM worker contexts get nudged periodically as well, since extensions etc could depend on it, now or eventually. I don't see why the code couldn't still live in xpconnect.

Comment 57

[Igor Bukanov]

(In reply to comment #56)
> There's no real need for workers to get a periodic call to their
> branch callbacks any longer, as long as they're guaranteed to get one shortly
> after they've been paused or canceled.
>

But this is exactly what the patch from the comment 53 proposes: the periodic calls for DOM workers even if they do not need it. 
 
> It also needs to ensure that any other contexts that are not DOM contexts and
> not DOM worker contexts get nudged periodically as well, since extensions etc
> could depend on it, now or eventually. I don't see why the code couldn't still
> live in xpconnect.

If an extension would need such mechanism, I suspect that the need would be very different from the main thread and would not be covered by simple api to trigger the callback with the hardcoded period. Note also that with this bug and the bug 477367 fixed the only need for the periodic calls for the main thread would be to show a slow script dialog with a frequency like once per 10s.

That is why I suggest to wait with the generic mechanism until the need for it comes to existence. With the watchdog in dom/src the code would be much simpler and easy to tailor.

Comment 58

[Igor Bukanov]

(In reply to comment #52)
> Created an attachment (id=361040) [details]
> v21, replacing v19 (v20 was bogus)

In the patch it is not sufficient to just call JS_YieldRequest(cx) for the current context. Due to possibility of nested requests on *different* contexts it is necessary to suspend all the requests on the current thread. This happens in the browser but can also be triggered in js shell via mixture of scatter and evalcx.

Comment 59

[Andreas Gal :gal]

I fixed #54. Thanks!

I am not sure I understand #58. We periodically nudge all contexts across all threads that have a pending request so all of them will yield eventually.

Comment 60

[Andreas Gal :gal]

Attachment: v22 with #54 addressed

Comment 61

[Andreas Gal :gal]

Attachment: v22 with #54 addressed and jst's xpconnect patch merged in (use this patch to build the browser)

Comment 62

[Andreas Gal :gal]

Attachment: v22, js-engine only

Comment 63

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Igor and jst are correct, there's no longer a need for worker contexts to be poked periodically as long as the GC code continues to poke *every* context that is running in a request (otherwise workers will never yield).

Comment 64

[Johnny Stenback (:jst)]

(In reply to comment #57)
> (In reply to comment #56)
> > There's no real need for workers to get a periodic call to their
> > branch callbacks any longer, as long as they're guaranteed to get one shortly
> > after they've been paused or canceled.
> >
> 
> But this is exactly what the patch from the comment 53 proposes: the periodic
> calls for DOM workers even if they do not need it. 

Sure, but I don't see it hurting DOM workers in *any* way here...

> > It also needs to ensure that any other contexts that are not DOM contexts and
> > not DOM worker contexts get nudged periodically as well, since extensions etc
> > could depend on it, now or eventually. I don't see why the code couldn't still
> > live in xpconnect.
> 
> If an extension would need such mechanism, I suspect that the need would be
> very different from the main thread and would not be covered by simple api to
> trigger the callback with the hardcoded period. Note also that with this bug

I suspect that some extensions would be perfectly happy with this 1s nudge, and you're probably right that some extensions would need something else. But IMO those extensions can write their own code to handle whatever their need is, those who don't need anything else get the benefit, or get no extra pain, w/o any extra code.

> and the bug 477367 fixed the only need for the periodic calls for the main
> thread would be to show a slow script dialog with a frequency like once per
> 10s.
> 
> That is why I suggest to wait with the generic mechanism until the need for it
> comes to existence. With the watchdog in dom/src the code would be much simpler
> and easy to tailor.

I guess my point is that I don't see us nudging every running context once a second would be hurting anything, and it'd be more code and more work to filter out specific contexts, and for no obvious reason IMO. But maybe I'm wrong...

Comment 65

[Andreas Gal :gal]

For #58: igor, I looked the worker callback and each worker only yields its own context, which is exactly what we still do (except that you don't have to do it in the callback any more).

Comment 66

[Johnny Stenback (:jst)]

Attachment: v22 against mozilla-central.

I pushed this to the try server, look for kill-op-count.

Comment 67

[Johnny Stenback (:jst)]

(In reply to comment #58)
> (In reply to comment #52)
> > Created an attachment (id=361040) [details] [details]
> > v21, replacing v19 (v20 was bogus)
> 
> In the patch it is not sufficient to just call JS_YieldRequest(cx) for the
> current context. Due to possibility of nested requests on *different* contexts
> it is necessary to suspend all the requests on the current thread. This happens
> in the browser but can also be triggered in js shell via mixture of scatter and
> evalcx.

If we nest JS on different contexs w/o suspending the outer context while running stuff on the inner one, we can deadlock in many ways. bent found several of these cases spread throughout our code, I would argue that if there are still cases where we switch contexts w/o suspending the request we're in, we should fix the code that violates this rather than working around it here. Or did I misunderstand your comment here?

Comment 68

[Brendan Eich [:brendan]]

We could go through cx->thread->contextList to find all the contexts in this runtime on the same thread, and automatically yield or suspend them as needed. The engine tries to cope with nested request depth already, and we allow multiple contexts per thread. So it is conceivable to have the engine check, since any such bug (whether you blame embedding or engine) is a bad one (deadlock).

We still do not support multiple runtimes per thread AFAIK, although Igor helped the GC situation -- all the caches and the traceMonitor are for only one runtime. But this is not a problem for now, and I hope we can avoid a runtime per thread in the future (instead moving to thread-local GC heaps to help web workers and avoid all locking overhead in the allocator).

/be

Comment 69

[Andreas Gal :gal]

Attachment: v23 against TM

Removed elapsed time tracking from API as per discussion with jst and based on igor's critism. The existing DOM time tracking code seems to work just fine.

Comment 70

[Andreas Gal :gal]

Attachment: v23 against TM, with cosmetic fixes, ready for review

Comment 71

[Andreas Gal :gal]

v22 + jst's patch cycled green on all platforms (twice, I sent off a try-server run at the same time jst did).

I think v23 is ready for review. Workers still need some mild tweaking. Waiting for bent for that.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1234036865.1234039313.12371.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1234039306.1234041754.16007.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1234038665.1234041435.15504.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1234041426.1234044195.20249.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1234039876.1234044086.20036.gz

Comment 72

[Andreas Gal :gal]

Note: v23 can't land by itself since it changes the JS API. We have to land it with jst's patch together.

Comment 73

[Andreas Gal :gal]

This patch yields a 20ms speedup on SS total in the shell for me.

Comment 74

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 361092 [details] [diff] [review]
v23 against TM, with cosmetic fixes, ready for review

Andreas, this patch still triggers opcallback on every context during GC, not just the ones running. Can we use the requestcontextiterator instead?

Comment 75

[Brendan Eich [:brendan]]

Comment on attachment 361092 [details] [diff] [review]
v23 against TM, with cosmetic fixes, ready for review

> JS_PUBLIC_API(void)
>+JS_SetOperationCallback(JSContext *cx, JSOperationCallback callback)
>+{
>     cx->operationCallback = callback;

Return the old callback, as advertised in the m.d.t.js-engine post and as is done with other such APIs, to relieve callers who need to save and restore of having to do a Get before Set.

>+++ b/js/src/jsapi.h
>@@ -563,6 +563,18 @@
> extern JS_PUBLIC_API(JSContextCallback)
> JS_SetContextCallback(JSRuntime *rt, JSContextCallback cxCallback);
> 
>+#ifndef JS_THREADSAFE
>+/*
>+ * This API will call the heartbeat callback at certain intervals. No
>+ * further guarantees are given.

Say more, in particular how JS_TriggerOperationCallback can be called from hbCallback.

> /*
>+ * These functions allow setting an operation callback that will be called
>+ * from the thread the context is associated some time after any thread
>+ * triggered the callback using JS_TriggerOperationCallback(cx).

There are comments in the .cpp files about API rules and constraints that really belong here in the .h file if not in the MDC docs. Start with moving those comments here. MDC can wait for things to solidify and we can evac the prose from jsapi.h to the wiki then.


>+#ifdef JS_THREADSAFE
>+JS_FRIEND_API(JSContext *)
>+js_RequestContextIterator(JSRuntime *rt, JSContext *prev)
>+{
>+    for (JSCList *link = prev ? prev->link.next : rt->contextList.next;
>+         link != &rt->contextList;
>+         link = link->next)
>+    {

Left brace can go on previous line; some style variance here, it's a hard case, but looks like normal brace style works here.

>+js_InvokeOperationCallback(JSContext *cx)
> {
>+    JS_ASSERT(cx->operationCallbackFlag);
>+    
>+    /*
>+     * Reset the callback flag first, then yield. If another thread is racing us
>+     * here we will accumulate another callback request which will be serviced
>+     * at the next opportunity.
>+     */
>+    cx->operationCallbackFlag = 0;
> 
>+    /*
>+     * We automatically yield the current context every time the operation
>+     * callback is hit since we might be called as a result of an impending
>+     * GC, which would deadlock if we do not yield. Operation callbacks
>+     * are supposed to happen rarely (seconds, not milliseconds) so it is
>+     * acceptable to yield at every callback.
>+     */
>+    JS_YieldRequest(cx);
> 
>     JSOperationCallback cb = cx->operationCallback;
> 
>+    /*
>+     * Important: Additional callbacks can occur inside the callback handler if it
>+     * re-enters the JS engine. The embedding must ensure that the callback is
>+     * disconnected before attempting such re-entry.

This is badly wrapped past column 80 -- compare to above. It wraps ideally with vim tw=79 Q2j cmd:

     * Important: Additional callbacks can occur inside the callback handler if
     * it re-enters the JS engine. The embedding must ensure that the callback
     * is disconnected before attempting such re-entry.

The above comments all have jsapi.h/MDC-dev-doc fodder. Perhaps some implementation or design-decision content could be left out, but seems like important stuff in general to convey to embedders.

>+
>+    /*
>+     * Notify all operation callbacks, which will give them a chance to
>+     * yield their current request. Contexts that are not currently
>+     * executing will perform their callback at some later point,
>+     * which then will be unnecessary, but harmless.
>+     */

     * Notify all operation callbacks, which will give them a chance to yield
     * their current request. Contexts that are not currently executing will
     * perform their callback at some later point, which then will be
     * unnecessary, but harmless.

>+    js_NudgeOtherContexts(cx);

> /*
>+ * Notify all operation callbacks, which will give them a chance to yield
>+ * their current request. Contexts that are not currently executing will 
>+ * perform their callback at some later point, which then will be 
>+ * unnecessary, but harmless.
>+ */
>+void
>+js_NudgeOtherContexts(JSContext *cx)

No need to restate the comment both places. Also, should this go in the .h file?

>+{
>+    JSContext *acx, *iter = NULL;
>+    JSRuntime *rt = cx->runtime;
>+    while ((acx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL)
>+        if (cx != acx && cx->requestDepth)
>+            JS_TriggerOperationCallback(acx);

Brace multiline body/condition statements (body or condition multiline -> brace the body).

It's old-C-style but a blank line after the declarations, especially with noisy initializers cluttering things up, makes this more readable.

>+js_NudgeThread(JSContext *cx, JSThread *thread)
>+{
>+    JSContext *acx, *iter = NULL;
>+    JSRuntime *rt = cx->runtime;
>+    while ((acx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL)
>+        if (cx != acx && cx->requestDepth && cx->thread == thread)
>+            JS_TriggerOperationCallback(acx);

Ditto.

>+        LIns* x = NULL;

Nit: blank line here.

>+        /*
>+         * In a single-thread build we count on the heartbeat counter and side exit
>+         * when it reaches its threshold point.
>+         * 
>+         * In a multi-threaded build we only poll the operation callback request
>+         * flag. It is updated asynchronously whenever the callback is to be invoked.
>+         */

Suggest wording and format tweaks:

        /*
         * In a single-thread build we sample on the heartbeat counter and side
         * exit when it reaches its threshold point.
         * 
         * In a multi-threaded build we poll the operation callback request
         * flag. It is updated asynchronously whenever the callback is to be
         * invoked.
         */

>@@ -4286,6 +4302,14 @@
>         if (innermostNestedGuard)
>             return js_AttemptToExtendTree(cx, innermostNestedGuard, lr, NULL);
>         return false;
>+#ifndef JS_THREADSAFE
>+      case TIMEOUT_EXIT: {
>+        JSRuntime* rt = cx->runtime;
>+          if (rt->hbCallback)

The 'if' is overindented by two spaces.

I skimmed the shell changes, got upvar business and it's late. Will r+ new patch.

/be

Comment 76

[Andreas Gal :gal]

Attachment: v24 with brendan's nits and change js_RequestContextIterator's signature to match js_ContextIterator

Comment 77

[Andreas Gal :gal]

Attachment: updated patch for new watchdog thread in browser, remove yield from workers

Comment 78

[Andreas Gal :gal]

as for #74 I was checking for requestDepth != 0 already, but I use the RequestContextIterator now. A new try server build is on the way. If that succeeds I will flag the patch for review. 

bent, do the workers need more work or are we good with the browser patch as is?

Comment 79

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Workers patch, v1

Patch for workers.

We now keep a list of the JSContexts that are created so that we can trigger their operation callbacks whenever we need to pause/resume/cancel. Callback is also triggered before running a worker for the first time so that we can suspend or cancel correctly. Applies on top of the spidermonkey patch and the browser patch.

Comment 80

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 361172 [details] [diff] [review]
updated patch for new watchdog thread in browser, remove yield from workers

jst, this doesn't seem to be used:

+    void RescheduleWatchdog(XPCContext* ccx);

Comment 81

[Andreas Gal :gal]

Attachment: v25, expose JS_RequestContextIterator as public API

patch passed try server

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1234128065.1234130509.16303.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1234129865.1234132655.19962.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1234130465.1234134690.23116.gz

Comment 82

[Igor Bukanov]

Comment on attachment 361187 [details] [diff] [review]
v25, expose JS_RequestContextIterator as public API

To Gal: could you use -p -U8 for patches intended for review?

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>+JS_PUBLIC_API(JSOperationCallback)
>+JS_SetOperationCallback(JSContext *cx, JSOperationCallback callback)
>+{

Assert here that CURRENT_THREAD_IS_ME(cx->thread).

>+    JSOperationCallback old = cx->operationCallback;
>     cx->operationCallback = callback;
>+    return old;
> }


IMO there is no point to return from the setter the old value. In most cases it has no use and in those cases when one need it, one also needs the getter. So it just adds code bloat.

>+#ifndef JS_THREADSAFE
>+/*
>+ * This API will call the heartbeat callback at certain intervals. No
>+ * guarantees are given how often the heartbeat will be called. In particular,
>+ * the heartbeat callbacks stop if the engine is completely idle and no
>+ * JS code is executed. The heartbeat callback can be used as a method of
>+ * last resort to nudge contexts into invoking their respective operation
>+ * callback via JS_TriggerOperationCallback(cx). This API should be avoided
>+ * whenever possible. Use a watchdog thread, alarm timer or a timer signal
>+ * instead to trigger the operation callbacks on a regular basis.
>+ */
>+extern JS_PUBLIC_API(JSHeartbeatCallback)
>+JS_SetHeartbeatCallback(JSRuntime *rt, JSHeartbeatCallback hbCallback);

I am not sure that this extra complexity with special callbacks is worth it. A simple JS_EnableHeartbeats(cx) to call the operation callback would cover all the cases. It would make the transition for older applications still using the branch callback semi-trivial and decreases the source complexity.

>+#ifdef JS_THREADSAFE
>+extern JS_PUBLIC_API(JSContext *)
>+JS_RequestContextIterator(JSRuntime *rt, JSContext **iterp);

It is not enough to expose JS_RequestContextIterator. As the function must be called with the GC lock taken, the engine must also expose the GC lock. Another observation is that this form with JSContext **iterp just complicates the code and using just JSContext *iterp should be enough. As a bonus the internal js_ContextIterator can also be simplified to use the same iteration form and assume that the GC lock is always taken.

>diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
> JSBool
>-js_ResetOperationCount(JSContext *cx)
>+js_InvokeOperationCallback(JSContext *cx)
> {

>+    JS_YieldRequest(cx);

Wrap the yield call in JS_THREADSAFE brackets to avoid useless empty call with !JS_THREADSAFE.

> #ifndef JS_TRACER
>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
>--- a/js/src/jscntxt.h
>+++ b/js/src/jscntxt.h
>@@ -319,6 +319,14 @@
> 
>     /* Context create/destroy callback. */
>     JSContextCallback   cxCallback;
>+
>+#ifndef JS_THREADSAFE
>+    /* Heartbeat callback. */
>+    JSHeartbeatCallback hbCallback;
>+    jsuword             heartbeat;
>+    
>+#define JS_HEARTBEAT_MASK 0xfffff

This limit is way to high, use something like 4096.

>+#endif
> 
>     /* Garbage collector state, used by jsgc.c. */
>     JSGCChunkInfo       *gcChunkList;
>@@ -837,10 +845,10 @@
> 
> struct JSContext {
>     /*
>-     * Operation count. It is declared as the first field in the struct to
>-     * ensure the fastest possible access.
>+     * If this flag is set, we were asked to call back the operation callback
>+     * as soon as possible.
>      */
>-    volatile int32      operationCount;
>+    volatile jsint      operationCallbackFlag;
> 
>     /* JSRuntime contextList linkage. */
>     JSCList             link;
>@@ -950,12 +958,7 @@
>     /* Per-context optional error reporter. */
>     JSErrorReporter     errorReporter;
> 
>-    /*
>-     * Flag indicating that operationCallback stores the deprecated branch
>-     * callback.
>-     */
>-    uint32              branchCallbackWasSet   :    1;
>-    uint32              operationLimit         :    31;
>+    /* Branch callback. */
>     JSOperationCallback operationCallback;
> 
>     /* Interpreter activation count. */
>@@ -1202,6 +1205,15 @@
> extern JSContext *
> js_ContextIterator(JSRuntime *rt, JSBool unlocked, JSContext **iterp);
> 
>+#ifdef JS_THREADSAFE
>+/*
>+ * Iterate through contexts with active requests. The caller must be holding
>+ * rt->gcLock.
>+ */
>+extern JS_FRIEND_API(JSContext *)
>+js_RequestContextIterator(JSRuntime *rt, JSBool unlocked, JSContext **iterp);
>+#endif
>+
> /*
>  * JSClass.resolve and watchpoint recursion damping machinery.
>  */
>@@ -1353,72 +1365,19 @@
> #endif
> 
> /*
>- * Update the operation counter according to the given weight and call the
>- * operation callback when we reach the operation limit. To make this
>- * frequently executed macro faster we decrease the counter from
>- * JSContext.operationLimit and compare against zero to check the limit.
>- *
>+ * If the operation callback flag was set, call the operation callback.
>  * This macro can run the full GC. Return true if it is OK to continue and
>  * false otherwise.
>  */
>-#define JS_CHECK_OPERATION_LIMIT(cx, weight)                                  \
>-    (JS_CHECK_OPERATION_WEIGHT(weight),                                       \
>-     (((cx)->operationCount -= (weight)) > 0 || js_ResetOperationCount(cx)))
>+#define JS_CHECK_OPERATION_LIMIT(cx) \
>+    (!(cx)->operationCallbackFlag || js_InvokeOperationCallback(cx))

Why the macros do not update the heartbit here? They should as it essential for loops in the native code.


>diff --git a/js/src/jsdate.cpp b/js/src/jsdate.cpp
>+uint32 
>+js_IntervalNow() 
>+{
>+    return uint32(PR_IntervalToMilliseconds(PR_IntervalNow()));
>+}

This is no longer necessary, right?

>diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp
>+#ifdef JS_THREADSAFE
>+    if (!StartWatchdogThread(cx))
>+        return 1;
>+#endif

Why the watchdog is always started even when it is not necessary?

Comment 83

[Andreas Gal :gal]

This is the code gcc generations for a setter with and without returning the previous value. The difference is 1 instruction (2 bytes). I am not a huge believe in this API style either, but I don't think the code bloat is really a problem.

.globl _a
_a:
	pushl	%ebp
	movl	%esp, %ebp
	movl	8(%ebp), %edx
	movl	12(%ebp), %ecx
	movl	(%edx), %eax
	movl	%ecx, (%edx)
	leave
	ret
	.align 4,0x90
.globl _b
_b:
	pushl	%ebp
	movl	%esp, %ebp
	movl	12(%ebp), %edx
	movl	8(%ebp), %eax
	movl	%edx, (%eax)
	leave
	ret
	.subsections_via_symbols

The heartbeat and context callbacks are different animals. The heartbeat has no guarantees whatsoever. The engine could use alarms internally in the future if we decide to go that route. I don't want to re-invent the current scheme with a new API. I want to get rid of the idea of guaranteed frequent callbacks. The heartbeat is a last-resort API. Hence also the infrequent callbacks. 4096 loops per tick causes a huge performance loss on tight loops. I really don't want to go there. I also don't want to send heartbeats from native code. If you need to guard native code, use a signal/alarm. I don't think anyone cares about this outside FF and we have our MT watchdog now.

I modeled the RequestContextIterator after the already existing ContextIterator API. I don't think it makes sense to break the API model, even though I agree with the general uselessness of the iterp** in the API call.

js_IntervalNow() is used by the shell. I wanted one unified API to simply the shell. Its useful enough to have in jsdate.cpp IMO. Its 17 bytes so overhead should be ok.

I will fix the remaining issues and upload a new version.

Comment 84

[Andreas Gal :gal]

sunspider scores:

ST shell+JIT, heartbeat disabled: 1074ms
ST shell+JIT, heartbeat enabled, 0xffffff mask: 1084ms
ST shell+JIT, heartbeat enabled, 0xffff mask: 1111ms

The numbers are a bit noisy but the trend is clear IMO.

Comment 85

[Andreas Gal :gal]

Attachment: v26, igor's nits minus discussion in #83

Comment 86

[Igor Bukanov]

(In reply to comment #83)
> I don't want to re-invent the current scheme with a
> new API.

This is exactly what the proposed API does. It even introduces a new type of callback that is not compatible with the existing code significantly complicating the porting efforts. The single JS_EnableHeartbit(cx) is enough.

> I also don't want to send heartbeats from native code. If you need to
> guard native code, use a signal/alarm.

So why bother with heartbit at all if it can not even guarantee that it will trigger the callback for all infinite loops? This is worse then simply saying to applications that from now you must use the watchdog threads/alarms. 

> I don't think anyone cares about this
> outside FF and we have our MT watchdog now.

Using the branch/operation callback to schedule the GC was the recommendation for many years. But even if you are right, then lets move that code to a separated bug. It won't affect FF so it can be landed to 1.9.1 after FF 3.1 release if SM users would complain.

> I modeled the RequestContextIterator after the already existing ContextIterator
> API. I don't think it makes sense to break the API model, even though I agree
> with the general uselessness of the iterp** in the API call.

JS_ContextIterator is broken by design in multi-threaded applications as it is very difficult to use it correctly without races/deadlocks. The problem is that it must be called outside the GC lock ensuring races with JS_(New|Destroy)Context() on other threads. On the other hand RequestContextIterator is intended to be safe and must be called with the GC lock held. So the more different it will be from JS_ContextIterator, the better.

I also do not know what to do about exposing the GC lock. Perhaps  RequestContextIterator should be just friend API to emphasis on "use on you own risk"?

> js_IntervalNow() is used by the shell. I wanted one unified API to simply the
> shell. Its useful enough to have in jsdate.cpp IMO. Its 17 bytes so overhead
> should be ok.

The shell can either use JS_Now or just PR_IntervalNow as the latter is only necessary for JS_THREADSAFE when the cost of JS_Now can be high in PR-locks.

Comment 87

[Brendan Eich [:brendan]]

JS_ContextIterator is ancient, it predates JS_THREADSAFE. We should not feel the need to follow it -- my review did not ask for that.

Igor makes a good point about JS_RequestContextIterator not being safely callable without the caller acquiring the GC lock. In this light giving it a differently styled name would be good. How about JS_NextRequestContext?

/be

Comment 88

[Brendan Eich [:brendan]]

(In reply to comment #86)
> (In reply to comment #83)
> > I don't want to re-invent the current scheme with a
> > new API.
> 
> This is exactly what the proposed API does. It even introduces a new type of
> callback that is not compatible with the existing code significantly
> complicating the porting efforts. The single JS_EnableHeartbit(cx) is enough.


> > I also don't want to send heartbeats from native code. If you need to
> > guard native code, use a signal/alarm.
> 
> So why bother with heartbit at all if it can not even guarantee that it will
> trigger the callback for all infinite loops? This is worse then simply saying
> to applications that from now you must use the watchdog threads/alarms. 

What iloops would not be caught? Native array methods were not monitored by the old operation callback to stop iloops, IIRC -- just to avoid a long time before the branch callback might run again.

We are not going to add such costly and impossible-to-calibrate overhead to native code. Asynchronous mechanisms are the new way, as the post to m.d.t.js-engine says. In this light, the heartbeat API is a minimal replaceent for the branch callback (not the old operation callback).

> > I don't think anyone cares about this
> > outside FF and we have our MT watchdog now.
> 
> Using the branch/operation callback to schedule the GC was the recommendation
> for many years. But even if you are right, then lets move that code to a
> separated bug. It won't affect FF so it can be landed to 1.9.1 after FF 3.1
> release if SM users would complain.

We're already in touch with embedders like MikeM and Wes, with notification via the newsgroup. We can certainly remove the heartbeat code from this bug's patch and leave nothing, but the heartbeat code was part of the newsgroup post. Why not let it ride for a bit?

> I also do not know what to do about exposing the GC lock. Perhaps 
> RequestContextIterator should be just friend API to emphasis on "use on you own
> risk"?

I missed this, it's spot on.

Andreas, please rename to js_NextRequestContext (lowercase js_ for friend APIs) to jscntxt.h and make it JS_FRIEND_API. We shouldn't be putting it in jsapi.h at all. Thanks.

> > js_IntervalNow() is used by the shell. I wanted one unified API to simply the
> > shell. Its useful enough to have in jsdate.cpp IMO. Its 17 bytes so overhead
> > should be ok.
> 
> The shell can either use JS_Now or just PR_IntervalNow as the latter is only
> necessary for JS_THREADSAFE when the cost of JS_Now can be high in PR-locks.

But what's good for the goose... Why not use the same approach and reduce ifdefs all around?

/be

Comment 89

[Brendan Eich [:brendan]]

(In reply to comment #88)
> (In reply to comment #86)
> > (In reply to comment #83)
> > > I don't want to re-invent the current scheme with a
> > > new API.
> > 
> > This is exactly what the proposed API does. It even introduces a new type of
> > callback that is not compatible with the existing code significantly
> > complicating the porting efforts. The single JS_EnableHeartbit(cx) is enough.

Lost my reply text here, sorry. I wrote something like:

We do not want to ease porting from branch callback based code, if the embedding can use signals or threads. The heartbeat is only for those embeddings that are single threaded and for some reason cannot use SIGALRM or equivalent.

Many embedders are downrev and stay that way for years, but with TraceMonkey more than a few are interested and willing to port in order to gain JITted performance. IMHO we should take advantage of this opportunity to break API compatibility selectively (not for little gain or across the board). The branch callback is a prime target.

/be

Comment 90

[Brendan Eich [:brendan]]

Fussing over the name. js_NextContextInARequest? The capitcal-A is ugly. js_NextActiveContext? Ambiguous about which list (thread->contextList or rt->contextList) is iterated. So, js_NextActiveContextInRuntime.

/be

Comment 91

[Brendan Eich [:brendan]]

(In reply to comment #90)
> Fussing over the name. js_NextContextInARequest? The capitcal-A is ugly.
> js_NextActiveContext? Ambiguous about which list (thread->contextList or
> rt->contextList) is iterated. So, js_NextActiveContextInRuntime.

But the leading rt parameter should make that clear. Ok, js_NextActiveContext, or js_NextRequestContext if adding another metaphor ("Active" for in-a-request) is one too many.

/be

Comment 92

[Andreas Gal :gal]

Attachment: v27

v27: I renamed js_RequestContextIterator to js_NextActiveContext as suggested by brendan and since we no longer strive for API compatibility I also removed the out parameter as suggested by igor.

Comment 93

[Damon Sicore (:damons)]

This looks like it blocks a P1.  Nominating.  Sayre?

Comment 94

[Igor Bukanov]

(In reply to comment #89)
> We do not want to ease porting from branch callback based code, if the
> embedding can use signals or threads.

Given the decision to remove the branch callback, lets consider its use cases to see how they can be replaced.

A typical case for the branch callback is to call JS_MaybeGC from it. That is no longer necessary as the same effect can be archived using JS_SetGCParameter(rt, JSGC_TRIGGER_FACTOR, 200) with small porting efforts.

Another case is to yield to allow for waiting GC to proceed on another thread. With the current patch it is done automatically so this use case for a branch callback is gone as well.

Thus what is left is terminating iloops. This is not relevant when the embedding controls the scripted code. If that is not possible but terminating iloops is desirable, we have 2 cases to consider.

1. JS_THREADSAFE embedding. With the patch the embedders are on they own and would need to implement their own mechanisms. But not all is bad since the shell code shows how to use a watchdog thread to terminate iloops even if one is forced to use undocumented API like accessing GC lock or internal enumerators over requests. 

2. Single threaded embedding. Here the patch provides explicit heartbit API. But those API are rather different in usage from the branch callback (per-runtime/versus per-context, very different call frequency). This would require non-trivial porting efforts to switch the old branch callback code. In addition, those API still cannot terminate iloops in the native code like Array(1<<27).sort() that represent practical infinity. Thus, instead of spending time to deal with heartbit API, the embedders would be better of with implementing alarm and signals.

Given this observation I think the heartbit API is rushed. IMO it would be better to have just code in js shell that shows how to install signals on all supported platforms to reliably terminate ilooping scripts. This would be similar in spirit to JS_THREADSAFE case and would not introduce yet another API to support.

Comment 95

[Andreas Gal :gal]

I am _more_ than happy to remove the heartbeat. Do we have consensus on this?

Comment 96

[Brendan Eich [:brendan]]

Lose the heartbit^H^H^Hbeat API.

/be

Comment 97

[Andreas Gal :gal]

Attachment: v28 with async timer in the ST shell (except for windows)

Windows does not currently support timeout in the ST shell. I will add that in a separate patch. I want to land this for the browser.

Comment 98

[Brendan Eich [:brendan]]

Comment on attachment 361409 [details] [diff] [review]
v28 with async timer in the ST shell (except for windows)

>+js_NextActiveContext(JSRuntime *rt, JSContext *cx)
>+{
>+    JSContext *iter = cx;
>+#ifdef JS_THREADSAFE
>+    while (((cx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL) &&
>+           !cx->requestDepth);

Prevailing style never writes iloops that way -- too easy to confuse with a rogue semicolon. Please use a continue; on its own line and brace it (cuz the condition is multiline), or else use an

        if (cx->requestDepth)
            break;

as the loop body (arguably better style).

>+js_NudgeOtherContexts(JSContext *cx)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JSContext *acx = NULL;
>+
>+    while ((acx = js_NextActiveContext(rt, acx)) != NULL) {

Do not set acx to null uselessly.

>+js_NudgeThread(JSContext *cx, JSThread *thread)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JSContext *acx = NULL;
>+    
>+    while ((acx = js_NextActiveContext(rt, acx)) != NULL) {

Ditto.

>@@ -3172,82 +3076,101 @@
> 
>     JS_LOCK_GC(rt);
>     while (gWatchdogThread) {
>+        JSContext *acx = NULL;
>+    
>+        while ((acx = js_NextActiveContext(rt, acx)))

Ditto.

>+WatchdogHandler(int sig)
>+{
>+    JSRuntime *rt = gRuntime;
>+    JSContext *acx = NULL;
>+    
>+    while ((acx = js_NextActiveContext(rt, acx)))

Ditto.

>+StartWatchdog(JSRuntime *rt)
>+{
>+    if (!gOperationLimit)
>+        return JS_TRUE;
>+
>+#ifndef XP_WIN
>+    alarm(1);
>+    signal(SIGALRM, WatchdogHandler); /* set the Alarm signal capture */

The old kernel hacker in me wants you to reorder these two lines.

r=me with these fixes. Please get r=igor too.

/be

Comment 99

[Andreas Gal :gal]

Attachment: v29, with windows timer code

Comment 100

[Andreas Gal :gal]

Attachment: v29, with windows timer code, this time for real

Comment 101

[Brendan Eich [:brendan]]

Comment on attachment 361434 [details] [diff] [review]
v29, with windows timer code, this time for real

>+js_NudgeOtherContexts(JSContext *cx)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JSContext *acx;
>+
>+    while ((acx = js_NextActiveContext(rt, acx)) != NULL) {

Whoops, and as you pointed out already on iChat, acx is used before set by the nested assignment -- restore the initializer, and sorry!

>+js_NudgeThread(JSContext *cx, JSThread *thread)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JSContext *acx;
>+    
>+    while ((acx = js_NextActiveContext(rt, acx)) != NULL) {

Ditto.

r=me with these fixes. rs=me (rubberstamp) on the Windows code, if it works, ship it. This can land in tracemonkey with my r+, I think we need to get other patches in and tested in that repo first. Igor's r+ still wanted.

/be

Comment 102

[Andreas Gal :gal]

Attachment: v30, r+/rs+ brendan, to land in TM, waiting for r+ from igor before going to mc

Comment 103

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 361172 [details] [diff] [review]
updated patch for new watchdog thread in browser, remove yield from workers

>@@ -1097,10 +1094,13 @@
>   if (debugPossible)
>     buttonFlags += nsIPrompt::BUTTON_TITLE_IS_STRING * nsIPrompt::BUTTON_POS_2;
> 
>+  ::JS_SetOperationCallback(cx, nsnull);

Nit: add a newline here.

Comment 104

[Johnny Stenback (:jst)]

Attachment: Updated watchdog thread for the browser patch

This has mrbkap's r+sr

Comment 105

[Andreas Gal :gal]

Attachment: combined patch, about to land on TM

Comment 106

[Andreas Gal :gal]

Combined patch landed on TM:

http://hg.mozilla.org/tracemonkey/rev/32fd75dcb1f7

Comment 107

[Mike Moening]

>IMHO we should take advantage of this opportunity to break API
>compatibility selectively (not for little gain or across the board). The branch
>callback is a prime target.
I agree, go ahead and break us. Those kind of changes are not a big deal. Just document the "proper" was of doing it well.  Some embedders are still on JS 1.5/1.6 and need to be dragged kicking and screaming into the future.

Comment 108

[Andreas Gal :gal]

Glad to hear Mike. We will let the new API bake a bit and then I will document the changes on the wiki that I had to do to adopt the shell to the new API. That should describe pretty much what other embedders have to do (its just a few lines, really).

Comment 109

[Igor Bukanov]

Comment on attachment 361436 [details] [diff] [review]
v30, r+/rs+ brendan, to land in TM, waiting for r+ from igor before going to mc

>diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp
> static void
> WatchdogMain(void *arg)
> {
>@@ -3172,82 +3076,123 @@
> 
>     JS_LOCK_GC(rt);
>     while (gWatchdogThread) {
>-        PRIntervalTime now = PR_IntervalNow();
>-        PRIntervalTime sleepDuration = PR_INTERVAL_NO_TIMEOUT;
>-        JSContext *iter = NULL;
>-        JSContext *acx;
>-
>-        while ((acx = js_ContextIterator(rt, JS_FALSE, &iter))) {
>-            if (acx->requestDepth > 0) {
>-                JSShellContextData *data = (JSShellContextData *)
>-                                           JS_GetContextPrivate(acx);
>-
>-                /*
>-                 * For the last context inside JS_DestroyContext the engine
>-                 * starts a new request to shutdown the runtime. For such
>-                 * context data is null.
>-                 */
>-                if (data)
>-                    CheckCallbackTime(acx, data, now, sleepDuration);
>-            }
>-        }
>-
>-        gLastWatchdogWakeup = now;
>-        gWatchdogSleepDuration = sleepDuration;
>+        JSContext *acx = NULL;
>+    
>+        while ((acx = js_NextActiveContext(rt, acx)))
>+            JS_TriggerOperationCallback(acx);
>+
> #ifdef DEBUG
>         PRStatus status =
> #endif
>-            PR_WaitCondVar(gWatchdogWakeup, sleepDuration);
>+        /* Trigger the operation callbacks every second. */
>+        PR_WaitCondVar(gWatchdogWakeup, PR_SecondsToInterval(1));

With a js shell left in a terminal window this will uselessly wakes up CPU each second. This consumes energy and gives an a example how not to program the wakeups. For example, Gnome developers recently spent quite an effort to eliminate loops like that from their code after it was shown how badly such wakeups can affect the battery time.

Instead the code should show the embedders how to do the scheduling with multiple threads and contexts so the watchdog wake ups only when the timeout expires, like it was before the patch. But that can go to another bug.

Comment 110

[Igor Bukanov]

Comment on attachment 361436 [details] [diff] [review]
v30, r+/rs+ brendan, to land in TM, waiting for r+ from igor before going to mc

>diff --git a/js/src/jsdate.h b/js/src/jsdate.h
>--- a/js/src/jsdate.h
>+++ b/js/src/jsdate.h
>@@ -119,6 +119,11 @@
> extern JS_FRIEND_API(jsdouble)
> js_DateGetMsecSinceEpoch(JSContext *cx, JSObject *obj);
> 
>+typedef uint32 JSIntervalTime;
>+
>+JSIntervalTime
>+js_IntervalNow();
>+

The patch must also declare PR_TicksPerSecond() as interval time may not be measured in milliseconds. But IMO this is useless duplication of code as in most cases the code can just use JS_Now().

Comment 111

[Andreas Gal :gal]

Now that we have an asynchronous notification model, the possibilities are endless. A patch to de-schedule the watchdog if all threads are waiting on I/O is very welcome.

As for js_IntervalNow I think the code does the right thing:

uint32
js_IntervalNow()
{
    return uint32(PR_IntervalToMilliseconds(PR_IntervalNow()));
}

#else /* !JS_THREADSAFE */

uint32
js_IntervalNow()
{
    return uint32(PRMJ_Now() / PRMJ_USEC_PER_MSEC);
}

But again, feel free to improve upon this.

Comment 112

[Andreas Gal :gal]

Looks like xpcshell doesn't like us. Investigating the failure. I am suspecting that we are triggering a totally unrelated bug here since something fails that we don't touch even remotely. More to follow.

http://hg.mozilla.org/tracemonkey/rev/35dc90d184e5

Comment 113

[Igor Bukanov]

(In reply to comment #112)
> Looks like xpcshell doesn't like us. Investigating the failure. I am suspecting
> that we are triggering a totally unrelated bug here since something fails that
> we don't touch even remotely. More to follow.
> 
> http://hg.mozilla.org/tracemonkey/rev/35dc90d184e5

The landed code unconditionally creates the watchdog thread in xpconnect. With the previous versions of the watchdog patch it also caused xpshell failures as for some unknown reasons some examples does not like it. The workaround was to create the thread lazily.

Comment 114

[Igor Bukanov]

Comment on attachment 361439 [details] [diff] [review]
Updated watchdog thread for the browser patch 

>diff -r 1b523edb7fef js/src/xpconnect/src/xpcjsruntime.cpp
>+//static
>+void
>+XPCJSRuntime::WatchdogMain(void *arg)
>+{
>+    XPCJSRuntime* self = static_cast<XPCJSRuntime*>(arg);
>+
>+    // Lock lasts until we return
>+    AutoLockJSGC lock(self->mJSRuntime);
>+
>+    while (self->mWatchdogThread)
>+    {
>+#ifdef DEBUG
>+        PRStatus status =
>+#endif
>+            PR_WaitCondVar(self->mWatchdogWakeup, PR_TicksPerSecond());

This wakes up the browser each second even when the browser is completely idle and runs no JS code - notebooks users would just love this battery eating feature. 

>@@ -1014,7 +1076,9 @@ XPCJSRuntime::XPCJSRuntime(nsXPConnect* 
>@@ -1068,6 +1133,12 @@ XPCJSRuntime::XPCJSRuntime(nsXPConnect* 
>     if(mJSRuntime && !JS_GetGlobalDebugHooks(mJSRuntime)->debuggerHandler)
>         xpc_InstallJSDebuggerKeywordHandler(mJSRuntime);
> #endif
>+
>+    AutoLockJSGC lock(mJSRuntime);
>+
>+    mWatchdogThread = PR_CreateThread(PR_USER_THREAD, WatchdogMain, this,
>+                                      PR_PRIORITY_NORMAL, PR_LOCAL_THREAD,
>+                                      PR_UNJOINABLE_THREAD, 0);

This creates the thread even when creation of mWatchdogWakeup and the runtime would fail leading to segfaults in the watchdog.

Comment 115

[Andreas Gal :gal]

I have narrowed down the failure in test_bogus_files.js a bit:

The test reports an exception due to an invalid URI. This is the manipulation of cx->fp along the exception throwing path without the patch:

*** test pending

Breakpoint 21, nsXPCComponents_Utils::Import (this=0x830e00, registryLocation=@0x8311b0) at ../../../../../js/src/xpconnect/src/xpccomponents.cpp:3644
3644	        do_GetService(MOZJSCOMPONENTLOADER_CONTRACTID);
(gdb) enable 22
(gdb) c
Continuing.
Hardware watchpoint 22: *(JSStackFrame **) 17211544

Old value = (JSStackFrame *) 0xbfffdb2c
New value = (JSStackFrame *) 0x0
JS_SaveFrameChain (cx=0x106a000) at ../../../js/src/jsapi.cpp:5448
5448	    return fp;
(gdb) c
Continuing.
Hardware watchpoint 22: *(JSStackFrame **) 17211544

Old value = (JSStackFrame *) 0x0
New value = (JSStackFrame *) 0xbfffdb2c
JS_RestoreFrameChain (cx=0x106a000, fp=0xbfffdb2c) at ../../../js/src/jsapi.cpp:5461
5461	    cx->dormantFrameChain = fp->dormantNext;
(gdb) c
Continuing.

Breakpoint 18, mozJSComponentLoader::ReportOnCaller (this=0x81b160, cc=0xbfffda30, format=0xa5ab0c "%s - EXPORTED_SYMBOLS is not an array.") at ../../../../../js/src/xpconnect/loader/mozJSComponentLoader.cpp:1621
1621	    if (!cc) {
(gdb) 

We arrive in ReportOnCaller with cx->fp restored to its original value.

With the patch applied we get the following:

*** test pending

Breakpoint 12, JS_SetPendingException (cx=0x106a000, v=8218944) at ../../../js/src/jsapi.cpp:5916
5916        CHECK_REQUEST(cx);
(gdb) c
Continuing.

Breakpoint 21, nsXPCComponents_Utils::Import (this=0x830f10, registryLocation=@0x8312c0) at ../../../../../js/src/xpconnect/src/xpccomponents.cpp:3642
3642            do_GetService(MOZJSCOMPONENTLOADER_CONTRACTID);
(gdb) enable 22
(gdb) c
Continuing.
Hardware watchpoint 22: *(JSStackFrame **) 17211544

Old value = (JSStackFrame *) 0xbfffdb2c
New value = (JSStackFrame *) 0x0
JS_SaveFrameChain (cx=0x106a000) at ../../../js/src/jsapi.cpp:5385
5385        return fp;
(gdb) c
Continuing.
Hardware watchpoint 22: *(JSStackFrame **) 17211544

Old value = (JSStackFrame *) 0x0
New value = (JSStackFrame *) 0xbfffdb2c
JS_RestoreFrameChain (cx=0x106a000, fp=0xbfffdb2c) at ../../../js/src/jsapi.cpp:5398
5398        cx->dormantFrameChain = fp->dormantNext;

(gdb) c
Continuing.
Hardware watchpoint 22: *(JSStackFrame **) 17211544

Old value = (JSStackFrame *) 0xbfffdb2c
New value = (JSStackFrame *) 0x0
JS_SaveFrameChain (cx=0x106a000) at ../../../js/src/jsapi.cpp:5385
5385        return fp;

Breakpoint 18, mozJSComponentLoader::ReportOnCaller (this=0x81b160, cc=0xbfffda30, format=0xa5ab0c "%s - EXPORTED_SYMBOLS is not an array.") at ../../../../../js/src/xpconnect\
/loader/mozJSComponentLoader.cpp:1621
1621        if (!cc) {
(gdb) n
1626        va_start(ap, format);
(gdb) n
1630        rv = cc->GetJSContext(&callerContext);
(gdb) n
1631        NS_ENSURE_SUCCESS(rv, rv);
(gdb) p callerContext.fp
$2 = (JSStackFrame *) 0x0

We arrive in ReportOnCaller with cx->fp == NULL due to an unbalanced JS_SaveFrameChain.

Comment 116

[Andreas Gal :gal]

The following part of bent's patch caused the regression:

diff --git a/js/src/xpconnect/loader/mozJSComponentLoader.cpp b/js/src/xpconnect/loader/mozJSComponentLoader.cpp
--- a/js/src/xpconnect/loader/mozJSComponentLoader.cpp
+++ b/js/src/xpconnect/loader/mozJSComponentLoader.cpp
@@ -1534,17 +1534,17 @@ mozJSComponentLoader::ImportInto(const n
         mod = newEntry;
     }
 
     NS_ASSERTION(mod->global, "Import table contains entry with no global");
     *_retval = mod->global;
 
     jsval symbols;
     if (targetObj) {
-        JSAutoRequest ar(mContext);
+        JSCLContextHelper cx(this);
 
         if (!JS_GetProperty(mContext, mod->global,
                             "EXPORTED_SYMBOLS", &symbols)) {
             return ReportOnCaller(cc, ERROR_NOT_PRESENT,
                                   PromiseFlatCString(aLocation).get());
         }
 
         JSObject *symbolsObj = nsnull;

bent, you owe me 5 hours of my life =) I will attempt to re-land the patch without this part, pending review by jst (will back out in case of failure or denied review).

Comment 117

[Andreas Gal :gal]

Patch landed pending approval by jst to drop the offending patch part.

http://hg.mozilla.org/tracemonkey/rev/08950e8b5254

Comment 118

[Andreas Gal :gal]

Attachment: patch without the JSCLContextHelper/JSAutoRequest change

Comment 119

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #118)
> patch without the JSCLContextHelper/JSAutoRequest change

Hm, that was to fix a deadlock where we switch contexts with a nested request, we can't just leave that as is. Looking at the code I don't see what is causing the problem:

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#1663

I guess we have to find the place where we're not restoring the chain?

Comment 120

[Igor Bukanov]

Comment on attachment 361493 [details] [diff] [review]
patch without the JSCLContextHelper/JSAutoRequest change

>diff --git a/js/src/jslock.cpp b/js/src/jslock.cpp
>--- a/js/src/jslock.cpp
>+++ b/js/src/jslock.cpp
>+/*
>+ * Notify all contexts that are currently in a request and execute on this
>+ * specific thread.
>+ */
>+void
>+js_NudgeThread(JSContext *cx, JSThread *thread)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JSContext *acx = NULL;
>+    
>+    while ((acx = js_NextActiveContext(rt, acx)) != NULL) {
>+        if (cx != acx && cx->thread == thread)
>+            JS_TriggerOperationCallback(acx);

I should have seen this before: the check should be acx->thread == thread, not cx->thread == thread.

Comment 121

[Andreas Gal :gal]

bent, its fairly easy to debug with the debug logs above.

I think what happens is that the destructor isn't called until after ReportOnCaller is complete, but ReportOnCaller dies because it doesn't like cp.fp being NULL. Maybe what you want is pull ReportOnCaller out of the scope and goto to it somewhere near the end and then the restoring happens first?

Comment 122

[Andreas Gal :gal]

Attachment: v31, with fix for #120 and changed JSCLContextHelper use as described in #121

Comment 123

[Andreas Gal :gal]

Attachment: v31 with small formating fix (whitespace only)

Comment 124

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 361546 [details] [diff] [review]
v31, with fix for #120 and changed JSCLContextHelper use as described in #121

I talked with jst about this and I think we have a better solution. Patch in a sec.

Comment 125

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for component loader

Andreas, does this work?

Comment 126

[Andreas Gal :gal]

After discussion with bent we will pull the mozComponentLoader deadlock issue out of this bug and land the patch without a fix for that (its unrelated to timeout). A separate bug will be filed to fix it. I will re-land the patch to fix the acx<->cx issue in #120.

Comment 127

[Andreas Gal :gal]

Attachment: v32, fix for #120 but removed JSCLContextHelper patch to mozComponentLoader which caused the xpcshell regression

Comment 128

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 361622 [details] [diff] [review]
v32, fix for #120 but removed JSCLContextHelper patch to mozComponentLoader which caused the xpcshell regression

Yeah. I'll file a separate bug on that.

Comment 129

[Andreas Gal :gal]

Landed in TM (again, 3rd time)

http://hg.mozilla.org/tracemonkey/rev/a58f611b061c

Comment 130

[Igor Bukanov]

(In reply to comment #129)
> Landed in TM (again, 3rd time)
> 
> http://hg.mozilla.org/tracemonkey/rev/a58f611b061c

Shall I file another bug for the second issue from the comment 114?

Comment 131

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #128)
> Yeah. I'll file a separate bug on that.

Bug 477924.

Comment 132

[Leon Sha]

Attachment: Add signal.h

There are build errors on solaris. I simply add #include signal.h.
"js.cpp", line 3172: Error: The function "signal" must have a prototype.
"js.cpp", line 3187: Error: The function "signal" must have a prototype.

Comment 133

[Andreas Gal :gal]

Comment on attachment 361724 [details] [diff] [review]
Add signal.h 

Sure. Can you please apply directly to the tracemonkey tree? We should bracket this in XP_UNIX. I am not sure what win32 has to say about signal.h.

Comment 134

[Leon Sha]

I did some search in mozilla tree, seems it is not necessary to add XP_UNIX for signal.h. For example, http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/threads/combined/pruthr.c#39.

Comment 135

[Andreas Gal :gal]

Good thinking. Push in the patch. You can watch the tinderboxes build it here:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=TraceMonkey

Comment 136

[Igor Bukanov]

(In reply to comment #13)
> As usual, patch is welcome.

Well, that have to wait little bit - I need to do other bugs first ;). My only worry is that we should not rush any new API here. For example, in the shell the code knows when it will suspend/resume and it is sufficient to put the watchdog pokes only on resumes when the watchdog sleeps forever. It does not require any help from JS runtime.

With DOM situation is somewhat similar. If the code would only care about the main thread, then the code can just wake up the watchdog only when the engine is about to run JS code on the main thread. This happens just in few places in dom/src/base/nsJSEnvironment. So no need for API here as well.

Comment 137

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/08950e8b5254

Comment 138

[Leon Sha]

http://hg.mozilla.org/tracemonkey/rev/f2cef256bf25

Comment 139

[Mike Moening]

Early adopter...pulled down the latest code.

JS_ClearOperationCallback() not longer exists.
JS_SetOperationCallback() no longer takes operationLimit parameter which we set to 200*4096.

We are using the callback to prevent long running runaway scripts and to kill them by returning JS_FALSE from the operation callback.

Can somebody provide documentation on how to do this in the new world?

Thanks!

Comment 140

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/a4d1fe147761aacb# has some information, not sure if you've already seen that thread.

Did anyone update MDC?

Comment 141

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/307c9a4a2f07

Comment 142

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3615ef2a7f37

Comment 143

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/423207fd3206

Comment 144

[Bob Clary [:bc] (inactive)]

js1_8_1/extensions/regress-477187.js
v 1.9.1, 1.9.2