Improve performance for xml parsing

alexander-schranz commented 3 years ago

I currently did analyse the performance of @sulu in https://github.com/sulu/sulu/pull/6089 after some optimizations on our side most time is spend now in xmlToColumns see the following benchmark:

Before

Response time ~1080ms

https://blackfire.io/profiles/af73cb80-e327-4d12-9e76-d729b7097b86/graph?settings%5Bdimension%5D=wt&settings%5Bdisplay%5D=landscape&settings%5BtabPane%5D=nodes&selected=&callname=main()

Apache Bench Test:

ab -n 50 -c 1 -l http://localhost/my-uri

Result: Requests per second: 1.01 [#/sec] (mean)

```bash Server Software: nginx/1.21.0 Server Hostname: stern-nl.localhost Server Port: 80 Document Path: /my-uri Document Length: 348158 bytes Concurrency Level: 1 Time taken for tests: 67.569 seconds Complete requests: 50 Failed requests: 0 Total transferred: 17429509 bytes HTML transferred: 17407759 bytes Requests per second: 0.74 [#/sec] (mean) Time per request: 1351.384 [ms] (mean) Time per request: 1351.384 [ms] (mean, across all concurrent requests) Transfer rate: 251.90 [Kbytes/sec] received Connection Times (ms) min mean[+/-sd] median max Connect: 0 0 0.0 0 0 Processing: 974 1351 479.1 1156 3550 Waiting: 973 1344 474.9 1155 3548 Total: 974 1351 479.1 1156 3550 Percentage of the requests served within a certain time (ms) 50% 1156 66% 1399 75% 1533 80% 1590 90% 1896 95% 2427 98% 3550 99% 3550 100% 3550 (longest request) ```

After

Response time ~580ms

Apache Bench Test:

ab -n 50 -c 1 -l http://localhost/my-uri

Result: Requests per second: 1.91 [#/sec] (mean)

```bash Server Software: nginx/1.21.0 Server Hostname: stern-nl.localhost Server Port: 80 Document Path: /my-uri Document Length: 348161 bytes Concurrency Level: 1 Time taken for tests: 30.322 seconds Complete requests: 50 Failed requests: 0 Total transferred: 17429472 bytes HTML transferred: 17407722 bytes Requests per second: 1.65 [#/sec] (mean) Time per request: 606.450 [ms] (mean) Time per request: 606.450 [ms] (mean, across all concurrent requests) Transfer rate: 561.33 [Kbytes/sec] received Connection Times (ms) min mean[+/-sd] median max Connect: 0 0 0.0 0 0 Processing: 504 606 88.9 571 789 Waiting: 503 605 88.8 570 789 Total: 504 606 88.9 571 789 Percentage of the requests served within a certain time (ms) 50% 571 66% 676 75% 693 80% 700 90% 729 95% 774 98% 789 99% 789 100% 789 (longest request) ```

https://blackfire.io/profiles/30ffb1b2-540d-4a7b-8831-68d3fe6c4564/graph?settings%5Bdimension%5D=wt&settings%5Bdisplay%5D=landscape&settings%5BtabPane%5D=nodes&selected=&callname=main()

This PR tries to use an more efficient way of parsing the xml structure sadly in costs of readability of the code, hope I can get into a form which it would be still maintainable.

The PR is not yet finished and I'm just experimenting here a little bit. The blackfire profiles seems currently not show the result I'm seeing in the response times, need to investigate here more time. Update added the apache bench benchmark results for better performance testing, it seems profiling did add additional overhead and so blackfire isnt the right tool here to check the real performance.

alexander-schranz commented 3 years ago

@dbu what do you think in general about changing the xml parsing here?

alexander-schranz commented 3 years ago

@dbu Thank you for the review will try to do the changes at the weekend!

alexander-schranz commented 3 years ago

@dbu Did now create a class and use it inside xmlToProps and xmlToColumns methods. Found some edge cases I didn't take into account (sv:property without sv:value, concated data values when having encoded string in the xml (foo & bar)). Basically they did fail in the whole test suit, still I created additional unit test for the new class as it was easier for testing. Pull Request is from my side now ready for review.

alexander-schranz commented 3 years ago

@dbu Code updated :)

dbu commented 3 years ago

https://github.com/jackalope/jackalope-doctrine-dbal/releases/tag/1.7.0

alexander-schranz commented 3 years ago

@dbu Thank you!

plozmun commented 3 years ago

Hi @alexander-schranz , i've tested this change and it is a great improvement. Thank you!!! 👏

alexander-schranz commented 3 years ago

@plozmun thank you for testing this out. I'm curious how much the performance difference it is in your case for uncached pages in prod env.

plozmun commented 3 years ago

@plozmun thank you for testing this out. I'm curious how much the performance difference it is in your case for uncached pages in prod env.

Frontend performance has improved over 70-80% for uncached pages

In the admin is where the improvement is huge, from having some timeout to responses in few seconds for sections with many pages.

alexander-schranz commented 3 years ago

@plozmun Wow 70-80% this is really great, would not have expected that 😲. So your timeouts issues are now gone? Which sulu version did you use to test it?

plozmun commented 3 years ago

@alexander-schranz yes! We have pages with many relations and 20 languages and those pages are where it really improves. Sulu version is 2.3

alexander-schranz commented 3 years ago

@plozmun Really great to hear that this did also solve your performance issues. Think with this stats you did use the 2.3@dev version wich also includes https://github.com/sulu/sulu/pull/6089 as this also improve the performance when loading nodes. Thank you for your feedback and testing this so out so early 🙏.

jackalope / jackalope-doctrine-dbal