Bug 28278 - Improve $KOHA_CONF parsing speed by using XML::LibXML
Summary: Improve $KOHA_CONF parsing speed by using XML::LibXML
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 7170 31393 28305 28385
  Show dependency treegraph
 
Reported: 2021-05-03 07:01 UTC by Julian Maurice
Modified: 2023-06-23 03:07 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 28278: Add unit test for Koha::Config::read_from_file (22.07 KB, patch)
2021-05-03 07:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 28278: Improve $KOHA_CONF parsing speed by using XML::LibXML (4.12 KB, patch)
2021-05-03 07:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 28278: Add unit test for Koha::Config::read_from_file (22.12 KB, patch)
2021-05-03 19:48 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28278: Improve $KOHA_CONF parsing speed by using XML::LibXML (4.18 KB, patch)
2021-05-03 19:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28278: Add unit test for Koha::Config::read_from_file (22.19 KB, patch)
2021-05-04 13:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28278: Improve $KOHA_CONF parsing speed by using XML::LibXML (4.24 KB, patch)
2021-05-04 13:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28278: Uncomment and test access_dirs (1.50 KB, patch)
2021-05-07 09:31 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2021-05-03 07:01:36 UTC

    
Comment 1 Julian Maurice 2021-05-03 07:03:12 UTC
Created attachment 120374 [details] [review]
Bug 28278: Add unit test for Koha::Config::read_from_file
Comment 2 Julian Maurice 2021-05-03 07:03:16 UTC
Created attachment 120375 [details] [review]
Bug 28278: Improve $KOHA_CONF parsing speed by using XML::LibXML

- Without the patch:

$ hyperfine --warmup 1 \
  'perl -MKoha::Config -e "Koha::Config->read_from_file(\$ENV{KOHA_CONF}) for (1..1000)"'

  Time (mean ± σ):      3.585 s ±  0.018 s    [User: 3.531 s, System: 0.049 s]
  Range (min … max):    3.547 s …  3.612 s    10 runs

- With the patch:

$ hyperfine --warmup 1 \
  'perl -MKoha::Config -e "Koha::Config->read_from_file(\$ENV{KOHA_CONF}) for (1..1000)"'

  Time (mean ± σ):      1.122 s ±  0.028 s    [User: 1.104 s, System: 0.014 s]
  Range (min … max):    1.095 s …  1.189 s    10 runs

Test plan:
1. Apply the first patch (the one with the unit tests) and make sure
   tests pass: `prove t/Koha/Config.t`
2. Apply the rest of the patches and verify that tests still pass:
   `prove t/Koha/Config.t`.
Comment 3 Victor Grousset/tuxayo 2021-05-03 19:48:59 UTC
Created attachment 120414 [details] [review]
Bug 28278: Add unit test for Koha::Config::read_from_file

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 4 Victor Grousset/tuxayo 2021-05-03 19:49:03 UTC
Created attachment 120415 [details] [review]
Bug 28278: Improve $KOHA_CONF parsing speed by using XML::LibXML

- Without the patch:

$ hyperfine --warmup 1 \
  'perl -MKoha::Config -e "Koha::Config->read_from_file(\$ENV{KOHA_CONF}) for (1..1000)"'

  Time (mean ± σ):      3.585 s ±  0.018 s    [User: 3.531 s, System: 0.049 s]
  Range (min … max):    3.547 s …  3.612 s    10 runs

- With the patch:

$ hyperfine --warmup 1 \
  'perl -MKoha::Config -e "Koha::Config->read_from_file(\$ENV{KOHA_CONF}) for (1..1000)"'

  Time (mean ± σ):      1.122 s ±  0.028 s    [User: 1.104 s, System: 0.014 s]
  Range (min … max):    1.095 s …  1.189 s    10 runs

Test plan:
1. Apply the first patch (the one with the unit tests) and make sure
   tests pass: `prove t/Koha/Config.t`
2. Apply the rest of the patches and verify that tests still pass:
   `prove t/Koha/Config.t`.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 5 Victor Grousset/tuxayo 2021-05-03 19:54:26 UTC
Works :)
Comment 6 Jonathan Druart 2021-05-04 09:21:44 UTC
How much is the gain then? 2.5s for 10000 runs?
Comment 7 Julian Maurice 2021-05-04 09:32:24 UTC
(In reply to Jonathan Druart from comment #6)
> How much is the gain then? 2.5s for 10000 runs?

It's for 1000 runs, so in the end I should gain something like 2.5ms at startup. It's not much, but still better, right ? :)

Note that this was tested on my laptop with a i5-6440HQ CPU @ 2.60GHz
The gain should be greater on slower hardware (just a guess; please test it on different hardware)
Comment 8 Jonathan Druart 2021-05-04 09:57:34 UTC
Right, the "10 runs" confused me, but ofc it's an average.
Comment 9 Jonathan Druart 2021-05-04 10:00:08 UTC
Without patch:
  Time (mean ± σ):      6.293 s ±  0.243 s    [User: 6.181 s, System: 0.090 s]
  Range (min … max):    6.012 s …  6.852 s    10 runs

With patch:
  Time (mean ± σ):      2.006 s ±  0.050 s    [User: 1.937 s, System: 0.058 s]
  Range (min … max):    1.939 s …  2.092 s    10 runs


i5-6200U CPU @ 2.30GHz
Comment 10 Martin Renvoize 2021-05-04 13:05:08 UTC
Without patch
  Time (mean ± σ):      7.349 s ±  0.268 s    [User: 6.451 s, System: 0.348 s]
  Range (min … max):    6.968 s …  7.775 s    10 runs

With patch
  Time (mean ± σ):      2.377 s ±  0.104 s    [User: 1.993 s, System: 0.148 s]
  Range (min … max):    2.266 s …  2.551 s    10 runs

Run on a production virtual server.

I'm happy with the result and the test is thorough... no regressions found.

I'd love to see the subsequent bugs to split out the zebra config and possibly move away from XML.. but for now, even putting the performance improvement aside, this is a great step in the right direction for dropping our reliance on XML::Simple.

Passing QA
Comment 11 Martin Renvoize 2021-05-04 13:05:50 UTC
Created attachment 120455 [details] [review]
Bug 28278: Add unit test for Koha::Config::read_from_file

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2021-05-04 13:05:53 UTC
Created attachment 120456 [details] [review]
Bug 28278: Improve $KOHA_CONF parsing speed by using XML::LibXML

- Without the patch:

$ hyperfine --warmup 1 \
  'perl -MKoha::Config -e "Koha::Config->read_from_file(\$ENV{KOHA_CONF}) for (1..1000)"'

  Time (mean ± σ):      3.585 s ±  0.018 s    [User: 3.531 s, System: 0.049 s]
  Range (min … max):    3.547 s …  3.612 s    10 runs

- With the patch:

$ hyperfine --warmup 1 \
  'perl -MKoha::Config -e "Koha::Config->read_from_file(\$ENV{KOHA_CONF}) for (1..1000)"'

  Time (mean ± σ):      1.122 s ±  0.028 s    [User: 1.104 s, System: 0.014 s]
  Range (min … max):    1.095 s …  1.189 s    10 runs

Test plan:
1. Apply the first patch (the one with the unit tests) and make sure
   tests pass: `prove t/Koha/Config.t`
2. Apply the rest of the patches and verify that tests still pass:
   `prove t/Koha/Config.t`.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Jonathan Druart 2021-05-07 09:31:43 UTC
Created attachment 120666 [details] [review]
Bug 28278: Uncomment and test access_dirs
Comment 14 Jonathan Druart 2021-05-07 12:41:45 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 15 Fridolin Somers 2021-05-11 14:32:09 UTC
Enhancement not pushed to 20.11.x
Comment 16 Dobrica Pavlinusic 2021-05-19 10:31:36 UTC
This patch broke ldap configuration - fix is submitted under Bug 28385 - ldap server configuration broken since migration from XML::Simple
Comment 17 David Cook 2023-06-23 03:07:17 UTC
Looks like this introduced a problem for Shibboleth config too but I have a patch on bug 31393 for that.