Bug 48002 - network-simulator.php makes for very slow layout tests
Summary: network-simulator.php makes for very slow layout tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-20 13:15 PDT by Michael Nordman
Modified: 2010-11-02 18:09 PDT (History)
1 user (show)

See Also:


Attachments
faster network simulator (6.32 KB, patch)
2010-10-20 16:13 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
faster network simulator (7.45 KB, patch)
2010-10-27 15:44 PDT, Michael Nordman
ap: review-
Details | Formatted Diff | Diff
faster network simulator (7.50 KB, patch)
2010-10-28 15:49 PDT, Michael Nordman
ap: review+
Details | Formatted Diff | Diff
faster network simulator (6.36 KB, patch)
2010-11-02 16:22 PDT, Michael Nordman
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Nordman 2010-10-20 13:15:57 PDT
The script simulates network errors with by forever redirecting back to requested url. Some layout tests make repeated use of this script in a single test run. On an unloaded system, each invocation can take many round trips and multiple seconds to trigger the desired error (depending on the underlying http stack). On a system trying to run tests in parallel, this slowness is more pronounced and timeouts result.

A few appcache layout tests use this script and at least one xhr test. In many (maybe all?) of the layout tests, an HTTP error would work as well as a network error for testing the logic, and would run a lot faster.
Comment 1 Alexey Proskuryakov 2010-10-20 13:22:32 PDT
This script simulates a disconnected network, as well as we can. Sending an error response would test something different.
Comment 2 Michael Nordman 2010-10-20 13:29:04 PDT
(In reply to comment #1)
> This script simulates a disconnected network, as well as we can. Sending an error response would test something different.

Yes, I see what it's doing and why. The fact remains that we have tests that timeout due to this.

I question whether we need to simulate a network error in all the places where this script is used. It looks like a http error would effectively test the logic that wants to be tested. I'm looking thru the appcache tests and noticing that simulated network errors are used thruout, but haven't really noticed (m)any tests that simulate a server error.

Also, I question whether this is as good of a simulation of a network error as can be constructed. I wonder if we can close the underlying connection from the server-side?
Comment 3 Alexey Proskuryakov 2010-10-20 13:47:30 PDT
> Yes, I see what it's doing and why. The fact remains that we have tests that timeout due to this.

Can that be fixed in chromium networking code? I don't think CFNetwork has any performance issues with redirects.

> I'm looking thru the appcache tests and noticing that simulated network errors are used thruout, but haven't really noticed (m)any tests that simulate a server error.

Yeah, we need more of the latter kind.

> Also, I question whether this is as good of a simulation of a network error as can be constructed. I wonder if we can close the underlying connection from the server-side?

I couldn't find a better way when making those tests. If there is a way to force Apache to rudely drop a connection, that would be a much better approximation indeed.
Comment 4 Michael Nordman 2010-10-20 14:07:17 PDT
(In reply to comment #3)
> > Yes, I see what it's doing and why. The fact remains that we have tests that timeout due to this.
> 
> Can that be fixed in chromium networking code? I don't think CFNetwork has any performance issues with redirects.

It functions fine, its just slow on this non-sensical content. I really don't want to modify the networking code for this.

> 
> > I'm looking thru the appcache tests and noticing that simulated network errors are used thruout, but haven't really noticed (m)any tests that simulate a server error.
> 
> Yeah, we need more of the latter kind.

I'd like to switch some of the former to be some of the latter, and retain some 'network error' cases in there too, but lean on them less.

> > Also, I question whether this is as good of a simulation of a network error as can be constructed. I wonder if we can close the underlying connection from the server-side?
> 
> I couldn't find a better way when making those tests. If there is a way to force Apache to rudely drop a connection, that would be a much better approximation indeed.

I don't know php very well, I didn't see an obvious way to close the connection? If I could get a reference to the underlying 'socket' there is a call to close the socket. 

Another approach could be to simulate a 'network error' could be with a malformed http message, maybe an excessively large header? I'll see if php lets me form a malformed header? It'd be nice if php let me produce the output w/o doing anything 'for me' in this case.
Comment 5 Michael Nordman 2010-10-20 14:21:47 PDT
> Another approach could be to simulate a 'network error' could be with a malformed http message, maybe an excessively large header? I'll see if php lets me form a malformed header? It'd be nice if php let me produce the output w/o doing anything 'for me' in this case.

Ah ha... at least the instance of php i'm running here let's me produce arbitrarily large headers!

        # Simulate a network error with an excessively large response header.
        $largeString = "";
        for ($x = 0; $x < 10000; $x++)
            $largeString .= "abcdefghijklmonpqrstuvwxyz0123456789";
        header('HTTP/1.1 200 OK');
        header('X-WayTooBig: ' . $largeString);
Comment 6 Michael Nordman 2010-10-20 16:13:26 PDT
Created attachment 71353 [details]
faster network simulator

as mentioned in the previous comments
Comment 7 Alexey Proskuryakov 2010-10-20 16:27:03 PDT
Which network back-ends did you test with? What happened with performance on non-chromium ones?
Comment 8 Alexey Proskuryakov 2010-10-20 16:29:14 PDT
Also, why do you think that changing an existing test in this way is a good idea? You can add a new one if you want to test this code path.
Comment 9 Michael Nordman 2010-10-20 16:32:23 PDT
(In reply to comment #7)
> Which network back-ends did you test with? What happened with performance on non-chromium ones?

I haven't gotten that far yet, only run with chrome's test_shell thusfar (that's why no cq setting). I'm working on patching this in on my mac and am watching what the try bots are doing with the uploaded patch.

(In reply to comment #8)
> Also, why do you think that changing an existing test in this way is a good idea? You can add a new one if you want to test this code path.

As mentioned in the previous comments, because there's little (or no) coverage for http errors triggering fallbacks in the tests. I believe there still are other tests where 'network errors' trigger fallbacks.
Comment 10 Michael Nordman 2010-10-21 15:31:05 PDT
Bah! Excessively large headers don't induce an error in safari(win), looks like the value is truncated (at least as displayed by the inspector).

Curious tidbits...

* chrome and firefox will follow 21 self-redirects before giving up
* safari(win) follows 17
* ie8, followed 800+ redirects before i gave up on it

... so i definitely can't change this self-redirect following behavior in the network stack
Comment 11 Michael Nordman 2010-10-22 14:49:17 PDT
A related chrome issue with some relevant history.
http://code.google.com/p/chromium/issues/detail?id=54717
Comment 12 Michael Nordman 2010-10-27 15:44:47 PDT
Created attachment 72099 [details]
faster network simulator

A smaller change to the script that does the trick.

        # Simulate a network error by replying with a non-sense response.
        header('HTTP/1.1 307 Temporary Redirect');
        header('Location: ' . $_SERVER['REQUEST_URI']); # redirect to self
        header('Content-Length: 1');
        header('Content-Length: 5', false); # multiple content-length headers
        echo "Intentionally incorrect response.";
Comment 13 Alexey Proskuryakov 2010-10-27 16:22:16 PDT
Comment on attachment 72099 [details]
faster network simulator

View in context: https://bugs.webkit.org/attachment.cgi?id=72099&action=review

Nice, but please don't change existing tests.

> LayoutTests/http/tests/resources/network-simulator.php:5
> +// offline, it simulates a network error with a non-sense response.

What's the difference between non-sense and nonsense? Most dictionaries don't seem to include this spelling.

> LayoutTests/http/tests/resources/network-simulator.php:66
> +        header('Content-Length: 1');
> +        header('Content-Length: 5', false); # multiple content-length headers

A good comment would explain why we have these lines (like, it helps some network back-ends detect the error faster).
Comment 14 Michael Nordman 2010-10-27 16:31:22 PDT
(In reply to comment #13)
> (From update of attachment 72099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72099&action=review
> 
> Nice, but please don't change existing tests.

Do you mean in this patch or at all?
Comment 15 Alexey Proskuryakov 2010-10-27 17:02:38 PDT
Well, I certainly agree it's a good idea to land a test using the new capability in the same patch.

It seems best and safest to add a new one.
Comment 16 Michael Nordman 2010-10-27 17:26:16 PDT
(In reply to comment #15)
> Well, I certainly agree it's a good idea to land a test using the new capability in the same patch.

There is no new capability?

Some details of how the error is simulated is changed is all. The layout test changes in this patch are strictly minor improvements (imho). In fallback.html, don't send two requests when one will do. In offline-access.htlm, delete some stale code.
Comment 17 Alexey Proskuryakov 2010-10-27 17:31:52 PDT
I was distracted, and somehow thought of your earlier patch, sorry.
Comment 18 Alexey Proskuryakov 2010-10-27 17:34:11 PDT
There was some reason why I made a separate canLoad() check, I'll try to remember it. Maybe it wasn't a good one.
Comment 19 Michael Nordman 2010-10-28 15:49:17 PDT
Created attachment 72250 [details]
faster network simulator

spelling and wordsmithing
Comment 20 Michael Nordman 2010-10-29 13:17:36 PDT
ping
Comment 21 Michael Nordman 2010-11-01 14:30:35 PDT
ping
Comment 22 Alexey Proskuryakov 2010-11-01 14:49:21 PDT
I asked, and you said there was no urgency. Why do you ping each working day? Is this super urgent now?
Comment 23 Michael Nordman 2010-11-01 14:55:34 PDT
(In reply to comment #22)
> I asked, and you said there was no urgency. Why do you ping each working day? Is this super urgent now?

It isn't super urgent, but the timeout problem does obscure actual test results, so I'd like to get it in so I can start seeing the actual results more clearly. It's a small change so I thought it would be easy to review.

Maybe I should have somebody else take a look?
Comment 24 Alexey Proskuryakov 2010-11-02 12:23:48 PDT
Comment on attachment 72250 [details]
faster network simulator

View in context: https://bugs.webkit.org/attachment.cgi?id=72250&action=review

OK. Please address the comments, and verify how much of a performance win the test change is.

> LayoutTests/ChangeLog:9
> +        multiple content-length headers. Chrome's network stacks consider that an

Typo: considers.

> LayoutTests/ChangeLog:11
> +        This avoids time outs due to using PHP via slow CGI on windows.

Typo: timeouts.

> LayoutTests/ChangeLog:21
> +        * http/tests/resources/offline-access.html:

This change is in http/tests/appcache/resources/offline-access-frame.html, not offline-access.html.

> LayoutTests/http/tests/appcache/fallback.html:57
> +        log("FAIL:Cannot load a URL from fallback namespace when network is enabled, ex = " + ex);

Missing space.

> LayoutTests/http/tests/appcache/fallback.html:59
>          hadError = true;
>      }

I remembered what the benefit from canLoad() was - it helped write more concise and readable code. It's much harder to read the new version of the test.

If you get measurable speedup from these changes, let's land them - but if it can't be measured, let's keep the test as is.

> LayoutTests/http/tests/appcache/fallback.html:149
> -    if (!canLoad(testURL)) {
> -        log("FAIL: Cannot load an URL from fallback namespace when network is disabled");
> -        hadError = true;
> -    } else if (load(testURL) != load("resources/simple.txt")) {
> -        log("FAIL: Loading an URL from fallback namespace when network is disabled returned unexpected response");
> +    try {
> +      if (load(testURL) != load("resources/simple.txt")) {

There is a slight difference here - an exception from loading resources/simple.txt would be attributed to loading testURL. Not a big deal.
Comment 25 Michael Nordman 2010-11-02 15:58:58 PDT
When using php via cgi it is measurable, 2 of the 3 urls given to load() are satisfied by a php script. Under normal cirucumstances each php request adds about 100ms.

I can eliminate the multiple try/catch blocks if you think that would help.

function load(url)
{
    try {
        var req = new XMLHttpRequest();
        req.open("GET", url, false);
        req.send("");
        return req.responseText;
    catch (ex) {
        log("FAIL: Cannot load " + url + ", ex = " + ex);
        hadError = true;
        return "";  // This value should not be expected as the responseText for a url presented to this function.
    }
}


    if (load(redirectURL) != "Hello, World!") {
        log("FAIL: Loaded the wrong content for a redirect to another origin when network is enabled");
        hadError = true;
    }
Comment 26 Alexey Proskuryakov 2010-11-02 16:05:25 PDT
That looks better to me, thanks!
Comment 27 Michael Nordman 2010-11-02 16:22:20 PDT
Created attachment 72763 [details]
faster network simulator

new patch with changes outlined above
Comment 28 Alexey Proskuryakov 2010-11-02 16:31:50 PDT
Comment on attachment 72763 [details]
faster network simulator

View in context: https://bugs.webkit.org/attachment.cgi?id=72763&action=review

> LayoutTests/ChangeLog:15
> +        3) Removed some unneeded code from offline-access.html. Appcache events are guaranteed

offline-access-frame.html here, too.
Comment 29 Michael Nordman 2010-11-02 18:09:42 PDT
Committed r71196: <http://trac.webkit.org/changeset/71196>