1377131 - Try to trigger collector slices at times which disturb page js less (at least with iframes loaded after the top level page has been loaded)

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

The idea is that if main page has been loaded already, better to not
block user input or refresh driver to be handled. And also give better chance for other idle callbacks to run.

a local test (using the current background parser timer) hints that speedometer could get around 5 points with this.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Will there be some timeout on this?  I assume we don't want to delay potentially forever, right?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Also, what does this mean if a site adds many iframes at once?  For example, twitter uses a lot of iframes for the entries in the timeline.  I'm just wondering if this could trigger some pathological behavior in sites like that.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

If you look at the bug which is blocking this, you see there is the timer.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Note, we would be using idle queue, so if there isn't anything else to process, we'd still create DOM for iframes asap.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Yea.  I only ask because it reminds me of the change I made to setTimeout() so timers always yielded to the main thread.  Some sites had problems because they were designed in a way where they needed to force this work through when the thread is busy.  Just wondering if sites like twitter with lots of iframes will handle this ok.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

I'm sure I'll see plenty of test failures. I do expect that we have tons of racy tests.
I think I need to still tweak this.

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4e9cda69589fef9d29ab04495e6dddc279bd8de
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=c4e9cda69589fef9d29ab04495e6dddc279bd8de
remote: recorded changegroup in replication log in 0.092s

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip4

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=52b460272a34a316446235ece5496c5553c34ac4
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=52b460272a34a316446235ece5496c5553c34ac4

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Grr, we have quite some broken tests.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: approach 2, v3

Very different approach. Trying to utilize the time from uri load to necko giving back data, and also start of parsing to parser actually serving data back to main thread.


remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0233ffbff21a3491745d437208790335b14dc0d
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=e0233ffbff21a3491745d437208790335b14dc0d

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8883046 [details] [diff] [review]
approach 2, v3

Is this crazy enough? With this we should be able to remove some of our explicit collector running in mochitest framework at least.
Note, if user is actually using the web page, this doesn't cause any extra collections.

I don't quite understand ReadyToTriggerExpensiveCollectorTimer anymore given what we do in different slices, so I just removed it.
CCRunnerFired for example isn't expensive, it is the usually very fast forgetSkippable. And GCTimerFired only triggers sInterSliceGCRunner, and does nothing else.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

And I very much understand the patch is rather controversial, when we in other cases have tried to move to use idle time. But idle time and benchmarks don't really work well together.

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 8883046 [details] [diff] [review]
approach 2, v3

Review of attachment 8883046 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsJSEnvironment.cpp
@@ +2216,5 @@
>  void
> +nsJSContext::MaybeRunNextCollectorSlice(nsIDocShell* aDocShell,
> +                                        JS::gcreason::Reason aReason)
> +{
> +  if (aDocShell &&  XRE_IsContentProcess()) {

nit: extra space before XRE.

@@ +2234,5 @@
> +            uint32_t lastEventTime = 0;
> +            vm->GetLastUserEventTime(lastEventTime);
> +            uint32_t currentTime =
> +              PR_IntervalToMicroseconds(PR_IntervalNow());
> +            if ((currentTime - lastEventTime) >

The idea of having the GC do some work when the main thread might be waiting around sounds okay (ideally, wouldn't Quantum DOM deal with this somehow by scheduling stuff then?), but having a special mode that only activates when a page has just been loaded but the user hasn't interacted with the browser doesn't feel to me like it would help actual users.

::: dom/base/nsJSEnvironment.h
@@ +104,5 @@
>    // Return the longest CC slice time since ClearMaxCCSliceTime() was last called.
>    static uint32_t GetMaxCCSliceTimeSinceClear();
>    static void ClearMaxCCSliceTime();
>  
> +  static void RunNextCollectorTimer(JS::gcreason::Reason aReason = JS::gcreason::DOM_WINDOW_UTILS);

Just make aReason mandatory and pass it in for the one existing call site.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Our current setup should be working well with real web when user is interacting with the browser and all (and in which case there is usually plenty of idle time), but it really doesn't work well with benchmarks which trigger page loads and generate garbage a lot faster.
That is the case I'm trying to fix here, at least for iframes. I need to still think cases like window.open etc.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Some background is also in http://logs.glob.uno/?c=content#c451100

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #12) 
> The idea of having the GC do some work when the main thread might be waiting
> around sounds okay (ideally, wouldn't Quantum DOM deal with this somehow by
> scheduling stuff then?),
I don't see how Quantum DOM would help any of this. In this case we have just one docgroup running and it doesn't have much idle time to run background operations, like collectors.

> but having a special mode that only activates when
> a page has just been loaded but the user hasn't interacted with the browser
> doesn't feel to me like it would help actual users.
Page hasn't been loaded yet. This stuff is triggered first when necko is told to load something and then when necko knows there is some data to parse as HTML (but before HTML parser has done anything with the data)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: approach 2, v4

Fixed the handling in parser and made a param non-optional.

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=51dca6324f21258d74fbc5d90f97771dd1732290
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=51dca6324f21258d74fbc5d90f97771dd1732290

mccr8, how strongly are you against this kind of setup? :)
I'm having hard time to find other options where collectors wouldn't disturb random benchmarks a lot.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: approach 2, v5

added missing 'explicit'

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

One thing to remind we already have similar, but at worse time running hack in AutoEntryScript::~AutoEntryScript(). That is calling MaybeGC while page is already being created in the main thread.

Comment 19

[Andrew McCreight [:mccr8]]

Comment on attachment 8883245 [details] [diff] [review]
approach 2, v5

This still feels questionable to me. I'll defer to what Boris thinks, once he is back from vacation. He should probably look at that docshell/view manager code anyways.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Could you explain a bit what feels questionable in particular?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

And sure, a docshell peer would need to take a look at docshell part and a parser peer the parser part.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #20)
> Could you explain a bit what feels questionable in particular?

...since the more I think of this, the less weird this feel. This is basically a better version of 
http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/dom/script/ScriptSettings.cpp#692

Comment 23

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #22)
> ...since the more I think of this, the less weird this feel. This is
> basically a better version of 

I think it is worse because it means that behavior of the benchmark is different from the behavior a user actually experiences.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

How so? Normally GC/CC run during idle time. So user doesn't notice them. But since benchmarks don't have much idle time, GC/CC run at random times affecting the results.

Comment 25

[Andrew McCreight [:mccr8]]

The code that is run is different depending on whether a benchmark is being run or not. Hopefully this will result in the same behavior as normal user behavior, but it isn't guaranteed in any way.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Sure. Right now it isn't guaranteed either.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8883245 [details] [diff] [review]
approach 2, v5

I'll try a bit less controversial patch.
Only run collector if its timer should have run, but hasn't because main thread has been busy. (but still check use activity etc.)

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: approach 2, v6

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b601483dc9793832e387526980f6037652fe3f54

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Attachment: iframe_gc_8.diff

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b749e533a0767616f3dc8e43237cba4ebfb455e
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=6b749e533a0767616f3dc8e43237cba4ebfb455e
remote: recorded changegroup in replication log in 0.126s

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v11

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/406dfa798dfdfcf570b231bb15cd330979fe6f42
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=406dfa798dfdfcf570b231bb15cd330979fe6f42

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8898351 [details] [diff] [review]
v11

Still trying to sell this.
We used to trigger GC/CC right after tick, this makes us do that occasionally right before, with deadline (deadline is new in this version). And only when user hasn't been interacting with the browser for awhile.

Comment 32

[Andrew McCreight [:mccr8]]

Comment on attachment 8898351 [details] [diff] [review]
v11

Review of attachment 8898351 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

Please make sure that this doesn't cause an increase in oranges in Win32 debug reftests.

You should get Boris or somebody to review nsJSContext::MaybeRunNextCollectorSlice() because I don't understand that code at all and don't know if it is reasonable or not. The rest looks okay to me. You also need a commit message that is explaining what you are trying to do and why.

::: docshell/base/nsDocShell.cpp
@@ +11614,5 @@
>    }
>    rv = aURILoader->OpenURI(aChannel, openFlags, this);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // We're about to load a new page and it may take time before necko

Should this be "Necko" not "necko"?

::: dom/base/nsJSEnvironment.cpp
@@ -2007,5 @@
>      return;
>    }
>  
>    if (sGCTimer) {
> -    if (ReadyToTriggerExpensiveCollectorTimer()) {

I added this because I was concerned about making tests that call into this take too long. Please be sure that this doesn't make reftests time out way more often.

::: js/public/GCAPI.h
@@ +111,5 @@
>      D(UNUSED2)                                  \
>      D(USER_INACTIVE)                            \
> +    D(XPCONNECT_SHUTDOWN)                       \
> +    D(DOCSHELL)                                 \
> +    D(PARSER)

Maybe call this HTML_PARSER? In the JS engine, "parser" would usually refer to parsing JS.

::: parser/html/nsHtml5StreamParser.cpp
@@ +991,5 @@
>      do_QueryInterface(mRequest, &rv);
>    if (threadRetargetableRequest) {
>      rv = threadRetargetableRequest->RetargetDeliveryTo(mEventTarget);
> +    if (NS_SUCCEEDED(rv)) {
> +      // Parser thread should be now ready to get data from necko and parse it

"Necko"? Also, below.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #32)
> ::: dom/base/nsJSEnvironment.cpp
> @@ -2007,5 @@
> >      return;
> >    }
> >  
> >    if (sGCTimer) {
> > -    if (ReadyToTriggerExpensiveCollectorTimer()) {
> 
> I added this because I was concerned about making tests that call into this
> take too long. Please be sure that this doesn't make reftests time out way
> more often.
But those timers aren't anymore expensive.
GCTimerFired just starts an idle runner and CCRunnerFired usually just runs forgetskippable
 
> ::: js/public/GCAPI.h
> @@ +111,5 @@
> >      D(UNUSED2)                                  \
> >      D(USER_INACTIVE)                            \
> > +    D(XPCONNECT_SHUTDOWN)                       \
> > +    D(DOCSHELL)                                 \
> > +    D(PARSER)
> 
> Maybe call this HTML_PARSER? In the JS engine, "parser" would usually refer
> to parsing JS.
Indeed

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8898351 [details] [diff] [review]
v11

bz, could you take a look at MaybeRunNextCollectorSlice.
I'm trying to make us run collector slices at better time when iframes are being loaded in a fast pace - but without disturbing top level page loads.
And if user is active, then better to not do anything special.
And give collectors only up to the next tick time to run themselves.
(We might want to do this all also for top level page loads, but I'm being conservative here first.)


After this I'll look at bug 1378092 again and also perhaps try to remove explicit GC call from InvokeInterruptCallback - hopefully fixing those would lead us to run GC a bit less often at bad times.

(bz is not accepting review requests atm)

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8898351 [details] [diff] [review]
v11

This seems fairly reasonable.  I just have two requests:

1) A decent commit message.  Comment 34 would be a good start, but doesn't describe the parser bits.

2) Some documentation on RunNextCollectorTimer and MaybeRunNextCollectorSlice explaining what they're for (at their declaration sites) and why they do what they do (esp. for MaybeRunNextCollectorSlice) at their implementation sites.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Attachment: iframe_gc_12.diff

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10405ae76f20
Try to trigger collector slices at times which disturb page js less (at least with iframes loaded after the top level page has been loaded), r=mccr8,bz

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/10405ae76f20

Comment 39

[Joel Maher ( :jmaher ) (UTC -8)]

and some performance improvements:
== Change summary for alert #9287 (as of September 06 2017 18:30 UTC) ==

Improvements:

  4%  kraken summary linux64 pgo e10s     1,439.42 -> 1,380.67
  3%  kraken summary linux64 opt e10s     1,488.06 -> 1,436.53
  3%  kraken summary windows10-64 pgo e10s1,473.51 -> 1,423.91

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9287