Bug 7170 - Remove use of XML::Simple
Summary: Remove use of XML::Simple
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P3 normal (vote)
Target Milestone: ---
Assignee: Galen Charlton
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 7119
  Show dependency treegraph
 
Reported: 2011-11-06 03:43 UTC by Ian Walls
Modified: 2019-04-28 22:51 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7170: Remove XML::Simple usage in favour of YAML config file (1.85 KB, patch)
2011-12-06 21:27 UTC, Ian Walls
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Walls 2011-11-06 03:43:43 UTC
XML::Simple is known to be very slow compared to other XML parsers.  It's used to parse the koha-conf.xml, which is loaded on every page, taking ~600ms on my localhost, so this is a performance hit.  These files currently use or reference XML::Simple:

./about.pl
./C4/Auth_with_ldap.pm
./C4/Context.pm
./C4/External/Amazon.pm
./C4/External/BakerTaylor.pm
./C4/External/Syndetics.pm
./C4/ILSDI/Services.pm
./C4/Installer/PerlDependencies.pm
./C4/Reports/Guided.pm
./C4/Search/PazPar2.pm
./C4/Search.pm
./C4/SIP/Sip/Configuration.pm
./C4/SIP/xmlparse.pl
./C4/XISBN.pm
./labels/label-create-xml.pl
./misc/bin/zebraqueue_daemon.pl
./misc/cronjobs/zebraqueue_start.pl
./opac/ilsdi.pl
./patroncards/create-pdf.pl
./patroncards/edit-layout.pl
./svc/authentication
./svc/bib
./svc/bib_profile
./svc/new_bib
Comment 1 Ian Walls 2011-11-06 03:44:50 UTC
XML::LibXML, XML::CompactTree::XS and XML::SAX::Simple are possible candidates... end output needs to be a reference to a hash of hashs ($context->{'var'}->{'othervar'})
Comment 2 Ian Walls 2011-11-07 03:06:00 UTC
My initial research has shown that a drop-in replacement for XML::Simple is hard to find.  However, if we could parse the input as YAML instead of XML, then we could use YAML::Syck to load the file and return the same data structure.

I've copied the <config> block of koha-conf.xml to koha-conf.yaml, and written a new subroutine to parse that, then modified C4::Context->new to detect the file type and use either XML or YAML according to the KOHA_CONF value.

My VERY INITIAL tests indicate 2 orders of magnitude improvement in speed on that portion of code; from hundreds of milliseconds to 1 or 2.  More research is needed to confirm these findings, but they're encouraging.
Comment 3 Chris Cormack 2011-11-07 03:24:12 UTC
You should also look into bug 6193 which provides the speed increase from not having to parse the koha-conf.xml. And is ready to go right now
Comment 4 Ian Walls 2011-12-06 21:27:10 UTC Comment hidden (obsolete)
Comment 5 Ian Walls 2011-12-06 21:33:44 UTC
Sorry for the delay in submitting this; it's been a long while to recover my footing post-KohaCon.

A koha-conf.yaml file can be created out of the <config> tag of koha-conf.xml using this stylesheet: http://yaml.org/xml/xml2yaml.xsl

This patch is proof-of-concept.  If this is a direction we chose to go, then we need to put the YAML file into the Makefile, so it will be auto-generated along with koha-conf.xml.
Comment 6 Paul Poulain 2011-12-08 13:32:17 UTC
I've tested ian script with misc/load_testing/benchmark_staff.pl, here are the results:
(1st column with XML, 2nd column with YAML. Numbers are in ms, less is better)
23709|22453
30049|30749
27487|26463
52924|50355
35922|29679
33168|30388

The improvement is surprisingly low.

Also note xsltproc xml2yaml.xsl koha-conf.xml returned nasty results, and I had to build the yaml manually, here is what it look like:
db_scheme: mysql
database: XXXXXXXX
hostname: localhost
port: 3306
user: XXXXXX
pass: XXXXXXXXXX
biblioserver: biblios
biblioservershadow: 1
authorityserver: authorities
authorityservershadow: 1
intranetdir: /home/paul/koha.dev/koha-community
opacdir: /home/paul/koha.dev/koha-community/opac
opachtdocs: /home/paul/koha.dev/koha-community/koha-tmpl/opac-tmpl
intrahtdocs: /home/paul/koha.dev/koha-community/koha-tmpl/intranet-tmpl
includes: /home/paul/koha.dev/koha-community/koha-tmpl/intranet-tmpl/prog/en/includes/
logdir: /home/paul/koha.dev/installs/lecannetdesmaures/var/log
install_log: home/paul/koha.dev/installs/lecannetdesmaures/misc/koha-install-log
useldapserver: 0

I also checked with NYTProf
* with YAML = Profile of mainpage.pl for 1.97s (of 2.58s), executing 201727 statements and 52079 subroutine calls in 253 source files and 63 string evals.
* with XML = Profile of mainpage.pl for 1.97s (of 2.60s), executing 201727 statements and 52079 subroutine calls in 253 source files and 63 string evals.

still investigating, there must be something wrong...
Comment 7 Paul Poulain 2011-12-08 14:18:55 UTC
(In reply to comment #6)
> I also checked with NYTProf
> * with YAML = Profile of mainpage.pl for 1.97s (of 2.58s), executing 201727
> statements and 52079 subroutine calls in 253 source files and 63 string evals.
> * with XML = Profile of mainpage.pl for 1.97s (of 2.60s), executing 201727
> statements and 52079 subroutine calls in 253 source files and 63 string evals.
> 
> still investigating, there must be something wrong...

OK, I was testing twice with XML (forgot to update env variable)
Here is the result:

* with YAML: Profile of mainpage.pl for 1.92s (of 2.37s), executing 144420 statements and 33523 subroutine calls in 246 source files and 62 string evals.
* with XML = Profile of mainpage.pl for 1.97s (of 2.60s), executing 201727
statements and 52079 subroutine calls in 253 source files and 63 string evals.

it result in a *big* lowering of the number of statements executed, but not a big change in the duration of the page.
If someone has an idea why...

Looking at the details of NYTProf, I see:
* 262ms is spent in XMLin (XML version)
* 1.20ms is spent in YAML::Syck::LoadFile (YAML version)
That's a 260ms difference, why don't we see it in the benchmark and/or in the total duration of mainpage reported by NYTProf
Comment 8 Paul Poulain 2011-12-09 14:31:59 UTC
For the record: i've tested XML::SAX::Simple, just replacing the use XML::Simple by use XML::LibXML::Simple  qw(XMLin);
1st you get an error :
Unrecognised option: suppressempty at /home/paul/koha.dev/koha-community/C4/Context.pm line 231

If you remove the suppressemptry =>'' at line 231, you get:
Bad arg length for Socket::pack_sockaddr_in, length is 0, should be 4 at /usr/lib/perl/5.12/Socket.pm line 217.

I haven't investigated more
Comment 9 Paul Poulain 2012-03-12 12:40:11 UTC
with koha-conf.xml being memcached now, I think this is no more interesting.
Plus the patch does not work, it would need much more work to be managed during install.

So marking "RESO WONTFIX"
Comment 10 Ian Walls 2012-04-10 15:53:45 UTC
I'm reopening this report, in it's boarder context of removing XML::Simple, rather than the specific context of a YAML config file.  Obsoleting patch.

XML::Simple is notoriously slow; if we've got to process any XML, we should be doing it with the most efficient and speedy parser we can get.
Comment 11 mathieu saby 2013-02-03 08:40:34 UTC
Hello
Does someone still work on this bug?
XML::Simple is still used by a lot of files.

Any change that could improve performances would be a good thing! (And yes, I know Plack and Memcached ;-) )

Using Yaml instead of XML in some cases (particulary for Koha-conf) would also be a GREAT thing ( some libraries don't want to use Memcached).

M. Saby
Rennes 2 University
Comment 12 Jared Camins-Esakov 2013-02-03 15:10:00 UTC
(In reply to comment #11)
> Hello
> Does someone still work on this bug?
> XML::Simple is still used by a lot of files.

I was actually thinking about working on this on my flight home tomorrow, actually. It depends how tired I am. In order to stay awake on flights I work on projects like this, that do not require any outside input.

> Any change that could improve performances would be a good thing! (And yes,
> I know Plack and Memcached ;-) )

Plack is not ready for production in my opinion, so I wasn't going to suggest it. Memcached is good, but not a silver bullet. For this, an mmap'd cache might be useful. I will experiment if I work on the problem.

> Using Yaml instead of XML in some cases (particulary for Koha-conf) would
> also be a GREAT thing ( some libraries don't want to use Memcached).

If we can get drastically better performance using a different XML parser I think I might prefer that to switching to YAML, even if the other parser requires some massaging of the output data structure.
Comment 13 Jared Camins-Esakov 2013-02-03 16:26:27 UTC
Also note that if you are not using XML::SAX::ExpatXS as your parser, you should be. You can do that by setting the environment variable XML_SIMPLE_PREFERRED_PARSER to 'XML::SAX::ExpatXS'.
Comment 14 Paul Poulain 2013-02-08 11:29:35 UTC
(In reply to comment #12)
> Plack is not ready for production in my opinion, so I wasn't going to
> suggest it.

Hi Jared,

Why are you saying Plack is not ready for production ? we've it for the OPAC since 2 years.
We're about to deploy it on staff interface at SAN-Ouest Provence. If there's some trouble I should expect, I prefer to know it ASAP !
Comment 15 Jared Camins-Esakov 2013-02-08 13:35:18 UTC
(In reply to comment #14)
> (In reply to comment #12)
> > Plack is not ready for production in my opinion, so I wasn't going to
> > suggest it.
> 
> Hi Jared,
> 
> Why are you saying Plack is not ready for production ? we've it for the OPAC
> since 2 years.
> We're about to deploy it on staff interface at SAN-Ouest Provence. If
> there's some trouble I should expect, I prefer to know it ASAP !

As I have said many, many times, there are still a lot of problems with Plack, especially on the staff client. I patch them as soon as I find them, of course, but libraries expect all the pages to Just Work. The largest problem I am having with running the staff client under Plack at this point are to do with holds. Many times when I try to delete a hold either manually or by filling it I get a MySQL error, and the hold gets stuck. I haven't debugged the problem enough to file a bug report yet, so my workaround has just been deleting all holds every so often. There is also the problem with staging MARC files for importing (I get a "parseerror" dialog every so often, but the upload works, and I still haven't figured out why). On top of that, almost every time I test a patch that touches a script I haven't used before, I have run into Plack problems.
Comment 16 Jared Camins-Esakov 2013-02-08 13:41:37 UTC
(In reply to comment #11)
> Hello
> Does someone still work on this bug?
> XML::Simple is still used by a lot of files.
> 
> Any change that could improve performances would be a good thing! (And yes,
> I know Plack and Memcached ;-) )
> 
> Using Yaml instead of XML in some cases (particulary for Koha-conf) would
> also be a GREAT thing ( some libraries don't want to use Memcached).

I did a lot of testing, and my conclusions were as follows:

1. YAML is much faster than XML.
2. XML::Simple with $ENV{'XML_SIMPLE_PREFERRED_PARSER'} == 'XML::SAX::ExpatXS' is probably the best option for parsing the XML. XML::LibXML::Simple is a more-or-less drop-in replacement, but it's not appreciably faster.
Comment 17 Paul Poulain 2016-06-04 11:19:15 UTC
Issue with XML::Simple still here in 16.05
the following files are still using it:
automate_check-in-out.pl:use XML::Simple;
C4/Reports/Guided.pm:use XML::Simple;
C4/SIP/xmlparse.pl:use XML::Simple qw(:strict);
C4/SIP/Sip/Configuration.pm:use XML::Simple qw(:strict);
C4/XISBN.pm:use XML::Simple;
C4/ILSDI/Services.pm:use XML::Simple;
C4/ILSDI/Services.pm:	use XML::Simple;
C4/External/BakerTaylor.pm:use XML::Simple;
C4/External/Syndetics.pm:use XML::Simple;
C4/Search/PazPar2.pm:use XML::Simple;
C4/Context.pm:use XML::Simple;
C4/Search.pm:use XML::Simple;
svc/new_bib:use XML::Simple;
svc/bib:use XML::Simple;
svc/bib_profile:use XML::Simple;
svc/import_bib:use XML::Simple;
svc/authentication:use XML::Simple;
about.pl:use XML::Simple;
misc/bin/connexion_import_daemon.pl:use XML::Simple;
test_BZ8089.pl:use XML::Simple;
opac/ilsdi.pl:use XML::Simple;
patroncards/create-pdf.pl:use XML::Simple;
patroncards/edit-layout.pl:use XML::Simple;
labels/label-create-xml.pl:use XML::Simple;
Comment 18 David Cook 2016-06-06 00:33:59 UTC
I'd love to see XML::Simple disappear...

As per the synopsis for XML::Simple itself says, "You really don't want to use this module in new code."

http://search.cpan.org/~grantm/XML-Simple-2.22/lib/XML/Simple.pm#SYNOPSIS
Comment 19 M. Tompsett 2016-07-08 02:37:13 UTC
I was looking in the svc ones and came up with this hackiness.

sub build_xml_output {
    my ($xml_hash, $root_name) = @_;

    my @xml_tags = keys %{$xml_hash};
    my @values = map { { 'key' => $_, 'value' => $xml_hash->{$_} } } @xml_tags;
    my $xml_values;
    foreach my $value (@values) {
        $xml_values->{$value->{key}} = $value->{value};
    }

    my $dom = XML::LibXML::Document->new( '1.0', q{} );
    $dom->setStandalone(1);
    my $root = $dom->createElement( $root_name );
    $dom->setDocumentElement( $root );
    foreach my $xml_tag (@xml_tags) {
        my $xml_value = $xml_values->{$xml_tag};

        my $text = $dom->createTextNode( "\n  " );
        $root->appendChild($text);

        my $element = $dom->createElement( $xml_tag );
        $root->appendChild($element);

        if ($xml_value =~ /\<datafield/ ) {
            my $parser = XML::LibXML->new();
            my $fragment = $parser->parse_balanced_chunk( $xml_value );
            $element->appendChild( $fragment );
        }
        else {
            $text = $dom->createTextNode( $xml_value );
            $element->appendTextNode($text);
        }
    }

    my $text = $dom->createTextNode( "\n" );
    $root->appendChild($text);

    my $xml_output = $dom->toString(0);
    print $xml_output;
}

You can see why XML::Simple with its XMLout method is desirable. :)
Comment 20 Chris Cormack 2016-07-10 06:24:09 UTC
> 
> You can see why XML::Simple with its XMLout method is desirable. :)

Except I work with the Author of XML::Simple and as David has already pointed out, he recommends no one uses it.
Comment 21 David Cook 2016-08-01 23:37:00 UTC
(In reply to M. Tompsett from comment #19)
> I was looking in the svc ones and came up with this hackiness.
> 
> You can see why XML::Simple with its XMLout method is desirable. :)

That does indeed appear very hacky. It doesn't need to be so hacky. I have quite a bit of experience with XML::LibXML, and it wouldn't be hard to write a solid predictable output function.
Comment 22 David Cook 2016-08-01 23:37:20 UTC
(In reply to Chris Cormack from comment #20)
> > 
> > You can see why XML::Simple with its XMLout method is desirable. :)
> 
> Except I work with the Author of XML::Simple and as David has already
> pointed out, he recommends no one uses it.

But this more than anything else.