Bug 51479 – Web Inspector: chromium: inspector's tests are quite slow especially at windows(Debug).

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

almost all inspector's tests become flaky on chromium Win(dbg) after switching to DRT.
I just discovered that we have two additional GC runs per each WebViewHost.
Test_shell version had no such calls.
These gc runs eat 15% of cpu time.

[Ilya Tikhonovsky]

Created attachment 77228 [details]
[patch] initial version

[Joseph Pecoraro]

This isn't my territory, but a bit of background can be found on:
http://trac.webkit.org/changeset/71928

> In test_shell, when we close a window, we first load about:blank
> and run GC to fire the destruction logic of the page.  In test_shell,
> this happens in ~TestShell.  In DRT, we manage virtual windows as
> WebViewHosts, so we need to replicate this logic in ~WebViewHost.

[Ilya Tikhonovsky]

(In reply to comment #2)
> This isn't my territory, but a bit of background can be found on:
> http://trac.webkit.org/changeset/71928
> 
> > In test_shell, when we close a window, we first load about:blank
> > and run GC to fire the destruction logic of the page.  In test_shell,
> > this happens in ~TestShell.  In DRT, we manage virtual windows as
> > WebViewHosts, so we need to replicate this logic in ~WebViewHost.


I may be wrong but historically there was only one TestShell object per test_shell process.
I added another instance of TestShell object for the Inspector's window when I did inspector's tests support in test_shell.

I think that is the reason why we had a gc run for each inspector tests in test_shell. 

Right now in chromium version of DRT we have double gc run for each layout test and four gc runs for each inspector test.
I think this is overkill.

[Eric Seidel (no email)]

Comment on attachment 77228 [details]
[patch] initial version

Do we know when these were added or why?

[Eric Seidel (no email)]

Unfortunately Tony is on vacation, otherwise I would really like to see his commentary here.

I guess we can try this for now and roll this out if he comes back and can explain why it's needed.

We have general timeout/crash trouble with inspector tests.  See my mail from an hour ago to webkit-dev about crashy inspector tests.

[Eric Seidel (no email)]

Comment on attachment 77228 [details]
[patch] initial version

rs=me.

[Ilya Tikhonovsky]

Comment on attachment 77228 [details]
[patch] initial version

Clearing flags on attachment: 77228

Committed r74544: <http://trac.webkit.org/changeset/74544>

[Ilya Tikhonovsky]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

(In reply to comment #3)
> (In reply to comment #2)
> > This isn't my territory, but a bit of background can be found on:
> > http://trac.webkit.org/changeset/71928
> > 
> > > In test_shell, when we close a window, we first load about:blank
> > > and run GC to fire the destruction logic of the page.  In test_shell,
> > > this happens in ~TestShell.  In DRT, we manage virtual windows as
> > > WebViewHosts, so we need to replicate this logic in ~WebViewHost.
> 
> I may be wrong but historically there was only one TestShell object per test_shell process.
> I added another instance of TestShell object for the Inspector's window when I did inspector's tests support in test_shell.

In test_shell, there's one TestShell per window.  In DRT, there's only one TestShell instance, so I moved the gc calls into ~WebViewHost to mirror the usage in test_shell.

I think there might have been some plugin tests that depended on this behavior, but if there were no test failures after this change, seems fine to me.