449109 - Wake up Native event loop while JS working

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: Proposed fix

Maybe it would be better to put this patch directly in bug 308158...
But I will start from here

This is experimental patch which works only for linux GTK, and makes UI mostly responsive  while Scripts running.

Comment 1

[Stuart Parmenter]

This could lead to some weird reentrantcy issues I believe, where by we're processing one event and then calling in to process another event before we're done with the event we're doing now.  

what does:
-const PRUint32 MAYBE_GC_OPERATION_WEIGHT = 5000 * JS_OPERATION_WEIGHT_BASE;
+const PRUint32 MAYBE_GC_OPERATION_WEIGHT = 500 * JS_OPERATION_WEIGHT_BASE;
do?

Comment 2

[Oleg Romashin (:romaxa)]

> processing one event and then calling in to process another event before we're
> done with the event we're doing now.  

Yep, that is very visible especially for window close event, where without special check we can process close-event, and destroy window while JS running...

But this check already existing in this patch 
+    nsCOMPtr<nsPIDOMWindow> win(do_QueryInterface(ctx->GetGlobalObject()));
+    // Check for existing docshell before possible window close event
+    PRBool docshell = (win && win->GetDocShell());
.........
+    if (docshell && !win->GetDocShell())
+      return JS_FALSE;

> 
> what does:
> -const PRUint32 MAYBE_GC_OPERATION_WEIGHT = 5000 * JS_OPERATION_WEIGHT_BASE;
> +const PRUint32 MAYBE_GC_OPERATION_WEIGHT = 500 * JS_OPERATION_WEIGHT_BASE;
> do?
> 
Currently Branch callback frequency very low, and it is not enough to make native UI responsive...
new value mostly fine for such services like yahoo,google... also UI responsive on sunspider and dromaeo...

Of course there are small JS performance regression, but it is almost not sensible if you are not touching UI elements (srollbars) during benchmark running.

Comment 3

[Oleg Romashin (:romaxa)]

I have made some additional testing and I think value 500 can be changed to 1000...

Comment 4

[Mike Shaver (:shaver emeritus)]

Re-entering here is pretty scary, but the responsiveness is definitely desirable.  Do we have anything from the sync XHR work that can be used to avoid content re-entrance?  Chrome re-entrance is less of a concern for me, since we can more easily work around it in our own code, even on a product-by-product basis.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The sync XHR code does not protect against reentering, timers and events do fire during syncloading and causes reentrancy (we have bugs on this). As far as I know this is a problem we haven't solved.

Comment 6

[Benjamin Smedberg]

Bug 340345, to be precise.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 332261 [details] [diff] [review]
Proposed fix

I don't think this is something we can do just like that, nor do I know that it's something we want to do in the electrolysis world where we'll have more than one process that can run JS simultaneously etc. Not to mention that this breaks our run to completion model and could invalidate all kinds of assumptions, internally and on webpages.

Comment 8

[Brendan Eich [:brendan]]

Don't break run-to-completion.

If you can prove the only nesting script do not re-enter the "world" of JS that is being used by already-entered code, i.e., the new control flow is completely isolated -- including DOM and browser-object effects -- then maybe. Sounds hard.

/be

Comment 9

[Oleg Romashin (:romaxa)]

With e10s we don't need this patch anymore, because our UI is not blocked by random web content (JS/Plugins) in e10s case.