Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase a lot (x4) boot sequence #181

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@lyrixx
Copy link
Member

lyrixx commented Apr 3, 2019

  1. Use symfony/http-client to get the status of webdriver process
  2. Do not wait an extra 1 s

The major point is the first one. I paste here the commit details:

With a simple PHPUnit setup and a 'regular' web page I got the following
numbers:

Before:

Time: 13.39 seconds, Memory: 6.00MB

After:

Time: 3.27 seconds, Memory: 6.00MB

I Profiler the page with blackfire.
And I noticed file_get_contents() was very slow.

I first updated the binary (see previous commit) but without success.

Then I tried to reproduce manually what happend. The chromedrive boot
really fast, and is directly available on HTTP. So there is something wrong
with file_get_contents(). I used tcpdump to debug it, and the chrome
driver do well its job. The issue is in PHP :(.

Since we have a nice HTTP client Symfony, let's use it !

Here is the tcpdump of the dial between php and the driver

1:66, ack 1, win 342, options [nop,nop,TS val 3026213891 ecr 3026213891],
length 65 E..u..@.@.............%+...sO......V.i.....
.`\..`\.GET /status HTTP/1.1 Host: 127.0.0.1:9515 Connection: close

16:05:44.854645 IP 127.0.0.1.9515 > 127.0.0.1.59044: Flags [P.], seq 1:237,
ack 66, win 342, options [nop,nop,TS val 3026213891 ecr 3026213891], length
236 E.. .x@.@..]........%+..O..........V.......
.`\..`\.HTTP/1.1 200 OK Content-Length:133 Content-Type:application/json;
charset=utf-8 Connection:close

{"sessionId":"","status":0,"value":{"build":{"version":"alpha"},"os":{"arch":"x86_64","name":"Linux","version":"4.15.0-46-generic"}}}
16:05:44.854651 IP 127.0.0.1.59044 > 127.0.0.1.9515: Flags [.], ack 237,
win 350, options [nop,nop,TS val 3026213891 ecr 3026213891], length 0
E..4..@.@.............%+....O......^.(.....
.`\..`\.

[< HANG A LOT HERE >]

16:05:54.864184 IP 127.0.0.1.59044 > 127.0.0.1.9515: Flags [F.], seq 66,
ack 237, win 350, options [nop,nop,TS val 3026223901 ecr 3026213891],
length 0 E..4..@.@.............%+....O......^.(.....
.`...`\.

EDIT

I initially thought it will increase only the very boot sequence, but actually it speeds up eveything.

With 3 tests:

Before:

Time: 42.06 seconds, Memory: 6.00MB
After:
Time: 8.96 seconds, Memory: 6.00MB

@lyrixx lyrixx requested review from nicolas-grekas and dunglas Apr 3, 2019

@@ -20,6 +20,8 @@
"php": ">=7.1",
"facebook/webdriver": "^1.5",
"symfony/browser-kit": "^4.0",
"symfony/contracts": "1.1.x-dev@dev",
"symfony/http-client": "4.3.x-dev@dev",

This comment has been minimized.

Copy link
@lyrixx

lyrixx Apr 3, 2019

Author Member

YOLO :) I will be better in one month!

@dunglas

dunglas approved these changes Apr 3, 2019

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Apr 3, 2019

@stof

This comment has been minimized.

Copy link
Member

stof commented Apr 3, 2019

@dunglas this would require dropping support for symfony/process 3.4 LTS. As we already have a working solution here, that would be too bad.

Install version 4.3 of a new component in an existing project works without issue. But for existing component, it is harder (projects which are still using symfony/symfony would require a upgrading the whole framework for that).

@stof

This comment has been minimized.

Copy link
Member

stof commented Apr 3, 2019

Thus, waiting for CLI output might be much harder to implement, as the webdriver spec does not require the runners to have a common output when ready (it does not even require using a CLI runner)

@navitronic

This comment has been minimized.

Copy link
Contributor

navitronic commented Apr 4, 2019

As an aside to the code in the PR, and more about the validity of the changes, I ported the http client changes from this PR to my 3.4 compatible fork and used Guzzle to provide the http client. Happy to report it reduces the time taken to run a suite of 106 tests from 5.96 minutes to 2.84 minutes.

So, thanks a heap for figuring this out 😁

@soyuka

soyuka approved these changes Apr 4, 2019

Copy link

soyuka left a comment

This is good 👍

@lyrixx lyrixx force-pushed the lyrixx:cheetah branch 2 times, most recently from 119acb3 to 021f3a3 Apr 16, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Apr 16, 2019

I removed the commit where I updated the binaries because it breaks the tests:

                                                                                                                                                                                              
1) Symfony\Component\Panther\Tests\ClientTest::testCookie with data set #1 (array('Symfony\Component\Panther\Tes...ntTest', 'createPantherClient'), 'Symfony\Component\Panther\Client')       
Facebook\WebDriver\Exception\UnrecognizedExceptionException: invalid argument: invalid 'domain'                                                                                               
  (Session info: headless chrome=73.0.3683.86)                                                                                                                                                
  (Driver info: chromedriver=2.46.628388 (4a34a70827ac54148e092aafb70504c4ea7ae926),platform=Linux 4.15.0-47-generic x86_64)                                                                  
                                                                                                                                                                                              
/home/gregoire/dev/github.com/symfony/panther/vendor/facebook/webdriver/lib/Exception/WebDriverException.php:158                                                                              
/home/gregoire/dev/github.com/symfony/panther/vendor/facebook/webdriver/lib/Remote/HttpCommandExecutor.php:326                                                                                
/home/gregoire/dev/github.com/symfony/panther/vendor/facebook/webdriver/lib/Remote/RemoteWebDriver.php:547                                                                                    
/home/gregoire/dev/github.com/symfony/panther/vendor/facebook/webdriver/lib/Remote/RemoteExecuteMethod.php:40                                                                                 
/home/gregoire/dev/github.com/symfony/panther/vendor/facebook/webdriver/lib/WebDriverOptions.php:55                                                                                           
/home/gregoire/dev/github.com/symfony/panther/src/Cookie/CookieJar.php:39                                                                                                                     
/home/gregoire/dev/github.com/symfony/panther/tests/ClientTest.php:252                                                                                                                        

This was not mandatory to make what I wanted ...

And I also squashed my commits

@lyrixx lyrixx force-pushed the lyrixx:cheetah branch 2 times, most recently from b3222ac to e2316e0 Apr 16, 2019

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Apr 17, 2019

Do we merge it now, or do we wait for the release of the HttpClient component?

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Apr 17, 2019

What could go wrong if we merge this right now?

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Apr 17, 2019

@lyrixx we'll have to update composer.json when 4.3 will be released, and we'll not be able to tag a release of Panther in the meantime. Not a big issue on my side.

Use symfony/client to get the status of webdriver process
With a simple PHPUnit setup and a 'regular' web page I got the following
numbers:

Before:

> Time: 13.39 seconds, Memory: 6.00MB

After:

> Time: 3.27 seconds, Memory: 6.00MB

I Profiler the page with [blackfire](https://blackfire.io/profiles/60e52757-9b36-4e95-b084-ef34fd83108e/graph).
And I noticed `file_get_contents()` was **very slow**.

I first updated the binary (see previous commit) but without success.

Then I tried to reproduce manually what happend. The chromedrive boot
really fast, and is directly available on HTTP. So there is something
wrong with `file_get_contents()`. I used tcpdump to debug it, and the
chrome driver do well its job. The issue is in PHP :(.

Since we have a nice HTTP client Symfony, let's use it !

Here is the tcpdump of the dial between php and the driver

```
16:05:44.854533 IP 127.0.0.1.59044 > 127.0.0.1.9515: Flags [P.], seq
1:66, ack 1, win 342, options [nop,nop,TS val 3026213891 ecr
3026213891], length 65
E..u..@.@.............%+...sO......V.i.....
.`\..`\.GET /status HTTP/1.1
Host: 127.0.0.1:9515
Connection: close

16:05:44.854645 IP 127.0.0.1.9515 > 127.0.0.1.59044: Flags [P.], seq
1:237, ack 66, win 342, options [nop,nop,TS val 3026213891 ecr
3026213891], length 236
E.. .x@.@..]........%+..O..........V.......
.`\..`\.HTTP/1.1 200 OK
Content-Length:133
Content-Type:application/json; charset=utf-8
Connection:close

{"sessionId":"","status":0,"value":{"build":{"version":"alpha"},"os":{"arch":"x86_64","name":"Linux","version":"4.15.0-46-generic"}}}
16:05:44.854651 IP 127.0.0.1.59044 > 127.0.0.1.9515: Flags [.], ack 237,
win 350, options [nop,nop,TS val 3026213891 ecr 3026213891], length 0
E..4..@.@.............%+....O......^.(.....
.`\..`\.

[< HANG A LOT HERE >]

16:05:54.864184 IP 127.0.0.1.59044 > 127.0.0.1.9515: Flags [F.], seq 66,
ack 237, win 350, options [nop,nop,TS val 3026223901 ecr 3026213891],
length 0
E..4..@.@.............%+....O......^.(.....
.`...`\.
```

@lyrixx lyrixx force-pushed the lyrixx:cheetah branch from e2316e0 to 598b214 Apr 18, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Apr 18, 2019

BTW, the tests suite use to take 12+ minutes. Now it take less than 5 minutes. Not bad :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.