[PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[Edwin Török]

Fix warnings, and delete some obsolete code.
oxenstored contained a hand-rolled GC to perform hash-consing:
this can be done with a lot fewer lines of code by using the built-in Weak module.

The choice of data structures for trees/tries is not very efficient: they are just
lists. Using a map improves lookup and deletion complexity, and replaces hand-rolled
recursion with higher-level library calls.

There is a lot more that could be done to optimize socket polling:
an epoll backend with a poll fallback,but API structured around event-based polling
would be better. But first lets drop the legacy select based code: I think every
modern *nix should have a working poll(3) by now.

This is a draft series, in need of more testing.

Edwin Török (6):
  tools/ocaml/libs/xc: Fix ambiguous documentation comment
  tools/ocaml/xenstored: fix deprecation warning
  tools/ocaml/xenstored: replace hand rolled GC with weak GC references
  tools/ocaml/xenstored: drop select based
  tools/ocaml/xenstored: use more efficient node trees
  tools/ocaml/xenstored: use more efficient tries

 tools/ocaml/libs/xc/xenctrl.mli               |  2 +
 tools/ocaml/xenstored/connection.ml           |  3 -
 tools/ocaml/xenstored/connections.ml          |  2 +-
 tools/ocaml/xenstored/disk.ml                 |  2 +-
 tools/ocaml/xenstored/history.ml              | 14 ----
 tools/ocaml/xenstored/parse_arg.ml            |  7 +-
 tools/ocaml/xenstored/{select.ml => poll.ml}  | 14 +---
 .../ocaml/xenstored/{select.mli => poll.mli}  | 12 +---
 tools/ocaml/xenstored/store.ml                | 49 ++++++-------
 tools/ocaml/xenstored/symbol.ml               | 70 +++++--------------
 tools/ocaml/xenstored/symbol.mli              | 22 ++----
 tools/ocaml/xenstored/trie.ml                 | 61 +++++++---------
 tools/ocaml/xenstored/trie.mli                | 26 +++----
 tools/ocaml/xenstored/xenstored.ml            | 20 +-----
 14 files changed, 98 insertions(+), 206 deletions(-)
 rename tools/ocaml/xenstored/{select.ml => poll.ml} (85%)
 rename tools/ocaml/xenstored/{select.mli => poll.mli} (58%)

-- 
2.25.1

Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[Christian Lindig]

I am going to look at this in more detail. In general, all of this are welcome changes. The main problem with select/poll is emulation of select behaviour which creates a lot of lists and consequently memory garbage at high frequency. This change is not yet addressing that but by dropping select paves the way to a more efficient implementation.

________________________________________
From: Edwin Torok
Sent: 14 August 2020 23:11
To: xen-devel@lists.xenproject.org
Cc: Edwin Torok; Christian Lindig; David Scott; Ian Jackson; Wei Liu
Subject: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

Fix warnings, and delete some obsolete code.
oxenstored contained a hand-rolled GC to perform hash-consing:
this can be done with a lot fewer lines of code by using the built-in Weak module.

The choice of data structures for trees/tries is not very efficient: they are just
lists. Using a map improves lookup and deletion complexity, and replaces hand-rolled
recursion with higher-level library calls.

There is a lot more that could be done to optimize socket polling:
an epoll backend with a poll fallback,but API structured around event-based polling
would be better. But first lets drop the legacy select based code: I think every
modern *nix should have a working poll(3) by now.

This is a draft series, in need of more testing.

Edwin Török (6):
  tools/ocaml/libs/xc: Fix ambiguous documentation comment
  tools/ocaml/xenstored: fix deprecation warning
  tools/ocaml/xenstored: replace hand rolled GC with weak GC references
  tools/ocaml/xenstored: drop select based
  tools/ocaml/xenstored: use more efficient node trees
  tools/ocaml/xenstored: use more efficient tries

 tools/ocaml/libs/xc/xenctrl.mli               |  2 +
 tools/ocaml/xenstored/connection.ml           |  3 -
 tools/ocaml/xenstored/connections.ml          |  2 +-
 tools/ocaml/xenstored/disk.ml                 |  2 +-
 tools/ocaml/xenstored/history.ml              | 14 ----
 tools/ocaml/xenstored/parse_arg.ml            |  7 +-
 tools/ocaml/xenstored/{select.ml => poll.ml}  | 14 +---
 .../ocaml/xenstored/{select.mli => poll.mli}  | 12 +---
 tools/ocaml/xenstored/store.ml                | 49 ++++++-------
 tools/ocaml/xenstored/symbol.ml               | 70 +++++--------------
 tools/ocaml/xenstored/symbol.mli              | 22 ++----
 tools/ocaml/xenstored/trie.ml                 | 61 +++++++---------
 tools/ocaml/xenstored/trie.mli                | 26 +++----
 tools/ocaml/xenstored/xenstored.ml            | 20 +-----
 14 files changed, 98 insertions(+), 206 deletions(-)
 rename tools/ocaml/xenstored/{select.ml => poll.ml} (85%)
 rename tools/ocaml/xenstored/{select.mli => poll.mli} (58%)

--
2.25.1

Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[Christian Lindig]

________________________________________
From: Edwin Torok
Sent: 14 August 2020 23:11
To: xen-devel@lists.xenproject.org
Cc: Edwin Torok; Christian Lindig; David Scott; Ian Jackson; Wei Liu
Subject: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

Fix warnings, and delete some obsolete code.
oxenstored contained a hand-rolled GC to perform hash-consing:
this can be done with a lot fewer lines of code by using the built-in Weak module.

The choice of data structures for trees/tries is not very efficient: they are just
lists. Using a map improves lookup and deletion complexity, and replaces hand-rolled
recursion with higher-level library calls.

There is a lot more that could be done to optimize socket polling:
an epoll backend with a poll fallback,but API structured around event-based polling
would be better. But first lets drop the legacy select based code: I think every
modern *nix should have a working poll(3) by now.

This is a draft series, in need of more testing.

Edwin Török (6):
  tools/ocaml/libs/xc: Fix ambiguous documentation comment
  tools/ocaml/xenstored: fix deprecation warning
  tools/ocaml/xenstored: replace hand rolled GC with weak GC references
  tools/ocaml/xenstored: drop select based
  tools/ocaml/xenstored: use more efficient node trees
  tools/ocaml/xenstored: use more efficient tries

 tools/ocaml/libs/xc/xenctrl.mli               |  2 +
 tools/ocaml/xenstored/connection.ml           |  3 -
 tools/ocaml/xenstored/connections.ml          |  2 +-
 tools/ocaml/xenstored/disk.ml                 |  2 +-
 tools/ocaml/xenstored/history.ml              | 14 ----
 tools/ocaml/xenstored/parse_arg.ml            |  7 +-
 tools/ocaml/xenstored/{select.ml => poll.ml}  | 14 +---
 .../ocaml/xenstored/{select.mli => poll.mli}  | 12 +---
 tools/ocaml/xenstored/store.ml                | 49 ++++++-------
 tools/ocaml/xenstored/symbol.ml               | 70 +++++--------------
 tools/ocaml/xenstored/symbol.mli              | 22 ++----
 tools/ocaml/xenstored/trie.ml                 | 61 +++++++---------
 tools/ocaml/xenstored/trie.mli                | 26 +++----
 tools/ocaml/xenstored/xenstored.ml            | 20 +-----
 14 files changed, 98 insertions(+), 206 deletions(-)
 rename tools/ocaml/xenstored/{select.ml => poll.ml} (85%)
 rename tools/ocaml/xenstored/{select.mli => poll.mli} (58%)

--
2.25.1

This all looks good - I left a small comment on one of the patches and I agree that this needs testing. I also wonder about compatibility with earlier OCaml releases that we support but I see no real obstacles.

-- 
Acked-by: Christian Lindig <christian.lindig@citrix.com>

Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[Edwin Torok]

On Mon, 2020-08-17 at 14:56 +0200, Christian Lindig wrote:
> This all looks good - I left a small comment on one of the patches
> and I agree that this needs testing. I also wonder about
> compatibility with earlier OCaml releases that we support but I see
> no real obstacles.
> 

I've developed the series using OCaml 4.08.1. I think the newest
feature I used was Map.update (OCaml 4.06, nearly 3 years ago).
Looking through https://repology.org/project/ocaml/versions I'm not
sure if we can require more than 4.05 though.
The README in Xen doesn't specify a minimum version, but configure
checks for >=4.02.

I can try to backport my series to OCaml 4.05 (to use Map.find_opt
instead of Map.update) and update the configure check to require 4.05.
It would be possible to backport even further to 4.02 by introducing
additional inefficiencies (Map.mem + Map.find would traverse the map
twice, and Map.find on its own would raise an exception on Not found,
which is more costly than returning None in Map.find_opt), I'd avoid
doing that.

Xen's CI from automation might need some updates to use latest stable
versions:
* Fedora 29 is EOL, should use at least Fedora 31
* Debian Jessie is EOL. Stretch is present, but Buster is missing

Best regards,
--Edwin

Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[Christian Lindig]

I see little reason to support old OCaml releases and requiring OCaml 4.06 would be fine with me but I assume that the project might have its own ideas about this.

________________________________________
From: Edwin Torok
Sent: 18 August 2020 08:28
To: Christian Lindig; xen-devel@lists.xenproject.org
Cc: Ian Jackson; dave@recoil.org; wl@xen.org
Subject: Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

On Mon, 2020-08-17 at 14:56 +0200, Christian Lindig wrote:
> This all looks good - I left a small comment on one of the patches
> and I agree that this needs testing. I also wonder about
> compatibility with earlier OCaml releases that we support but I see
> no real obstacles.
>

I've developed the series using OCaml 4.08.1. I think the newest
feature I used was Map.update (OCaml 4.06, nearly 3 years ago).
Looking through https://repology.org/project/ocaml/versions I'm not
sure if we can require more than 4.05 though.
The README in Xen doesn't specify a minimum version, but configure
checks for >=4.02.

I can try to backport my series to OCaml 4.05 (to use Map.find_opt
instead of Map.update) and update the configure check to require 4.05.
It would be possible to backport even further to 4.02 by introducing
additional inefficiencies (Map.mem + Map.find would traverse the map
twice, and Map.find on its own would raise an exception on Not found,
which is more costly than returning None in Map.find_opt), I'd avoid
doing that.

Xen's CI from automation might need some updates to use latest stable
versions:
* Fedora 29 is EOL, should use at least Fedora 31
* Debian Jessie is EOL. Stretch is present, but Buster is missing

Best regards,
--Edwin

Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[Andrew Cooper]

On 18/08/2020 10:25, Christian Lindig wrote:
> I see little reason to support old OCaml releases and requiring OCaml 4.06 would be fine with me but I assume that the project might have its own ideas about this.
>
> ________________________________________
> From: Edwin Torok
> Sent: 18 August 2020 08:28
> To: Christian Lindig; xen-devel@lists.xenproject.org
> Cc: Ian Jackson; dave@recoil.org; wl@xen.org
> Subject: Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code
>
> On Mon, 2020-08-17 at 14:56 +0200, Christian Lindig wrote:
>> This all looks good - I left a small comment on one of the patches
>> and I agree that this needs testing. I also wonder about
>> compatibility with earlier OCaml releases that we support but I see
>> no real obstacles.
>>
> I've developed the series using OCaml 4.08.1. I think the newest
> feature I used was Map.update (OCaml 4.06, nearly 3 years ago).
> Looking through https://repology.org/project/ocaml/versions I'm not
> sure if we can require more than 4.05 though.
> The README in Xen doesn't specify a minimum version, but configure
> checks for >=4.02.
>
> I can try to backport my series to OCaml 4.05 (to use Map.find_opt
> instead of Map.update) and update the configure check to require 4.05.
> It would be possible to backport even further to 4.02 by introducing
> additional inefficiencies (Map.mem + Map.find would traverse the map
> twice, and Map.find on its own would raise an exception on Not found,
> which is more costly than returning None in Map.find_opt), I'd avoid
> doing that.
>
> Xen's CI from automation might need some updates to use latest stable
> versions:
> * Fedora 29 is EOL, should use at least Fedora 31
> * Debian Jessie is EOL. Stretch is present, but Buster is missing

We're working on the CI loop.

As maintainer, it is ultimately Christian's choice to as to if/when to
bump the minimum versions.


As a general rule, we don't want to be sufficiently bleeding edge to
rule out in-use distros.  I have no idea if 4.06 is ok there, or whether
it is too new.  Then again, the Ocaml components are strictly optional
so it is perhaps less important.

Whatever happens WRT version, the configure change should occur before
changes in the code which would fail on older versions.

~Andrew

Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[Wei Liu]

On Tue, Aug 18, 2020 at 01:40:18PM +0100, Andrew Cooper wrote:
> On 18/08/2020 10:25, Christian Lindig wrote:
> > I see little reason to support old OCaml releases and requiring OCaml 4.06 would be fine with me but I assume that the project might have its own ideas about this.
> >
> > ________________________________________
> > From: Edwin Torok
> > Sent: 18 August 2020 08:28
> > To: Christian Lindig; xen-devel@lists.xenproject.org
> > Cc: Ian Jackson; dave@recoil.org; wl@xen.org
> > Subject: Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code
> >
> > On Mon, 2020-08-17 at 14:56 +0200, Christian Lindig wrote:
> >> This all looks good - I left a small comment on one of the patches
> >> and I agree that this needs testing. I also wonder about
> >> compatibility with earlier OCaml releases that we support but I see
> >> no real obstacles.
> >>
> > I've developed the series using OCaml 4.08.1. I think the newest
> > feature I used was Map.update (OCaml 4.06, nearly 3 years ago).
> > Looking through https://repology.org/project/ocaml/versions I'm not
> > sure if we can require more than 4.05 though.
> > The README in Xen doesn't specify a minimum version, but configure
> > checks for >=4.02.
> >
> > I can try to backport my series to OCaml 4.05 (to use Map.find_opt
> > instead of Map.update) and update the configure check to require 4.05.
> > It would be possible to backport even further to 4.02 by introducing
> > additional inefficiencies (Map.mem + Map.find would traverse the map
> > twice, and Map.find on its own would raise an exception on Not found,
> > which is more costly than returning None in Map.find_opt), I'd avoid
> > doing that.
> >
> > Xen's CI from automation might need some updates to use latest stable
> > versions:
> > * Fedora 29 is EOL, should use at least Fedora 31
> > * Debian Jessie is EOL. Stretch is present, but Buster is missing
> 
> We're working on the CI loop.
> 
> As maintainer, it is ultimately Christian's choice to as to if/when to
> bump the minimum versions.
> 
> 
> As a general rule, we don't want to be sufficiently bleeding edge to
> rule out in-use distros.  I have no idea if 4.06 is ok there, or whether
> it is too new.  Then again, the Ocaml components are strictly optional
> so it is perhaps less important.
> 
> Whatever happens WRT version, the configure change should occur before
> changes in the code which would fail on older versions.

Yes I would like to see the bump happen before applying a version of
this series too.

Wei.

> 
> ~Andrew

Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code

[Edwin Torok]

On Thu, 2020-08-27 at 09:41 +0000, Wei Liu wrote:
> On Tue, Aug 18, 2020 at 01:40:18PM +0100, Andrew Cooper wrote:
> > On 18/08/2020 10:25, Christian Lindig wrote:
> > > I see little reason to support old OCaml releases and requiring
> > > OCaml 4.06 would be fine with me but I assume that the project
> > > might have its own ideas about this.
> > > 
> > > ________________________________________
> > > From: Edwin Torok
> > > Sent: 18 August 2020 08:28
> > > To: Christian Lindig; xen-devel@lists.xenproject.org
> > > Cc: Ian Jackson; dave@recoil.org; wl@xen.org
> > > Subject: Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code
> > > 
> > > On Mon, 2020-08-17 at 14:56 +0200, Christian Lindig wrote:
> > > > This all looks good - I left a small comment on one of the
> > > > patches
> > > > and I agree that this needs testing. I also wonder about
> > > > compatibility with earlier OCaml releases that we support but I
> > > > see
> > > > no real obstacles.
> > > > 
> > > I've developed the series using OCaml 4.08.1. I think the newest
> > > feature I used was Map.update (OCaml 4.06, nearly 3 years ago).
> > > Looking through https://repology.org/project/ocaml/versions I'm
> > > not
> > > sure if we can require more than 4.05 though.
> > > The README in Xen doesn't specify a minimum version, but
> > > configure
> > > checks for >=4.02.
> > > 
> > > I can try to backport my series to OCaml 4.05 (to use
> > > Map.find_opt
> > > instead of Map.update) and update the configure check to require
> > > 4.05.
> > > It would be possible to backport even further to 4.02 by
> > > introducing
> > > additional inefficiencies (Map.mem + Map.find would traverse the
> > > map
> > > twice, and Map.find on its own would raise an exception on Not
> > > found,
> > > which is more costly than returning None in Map.find_opt), I'd
> > > avoid
> > > doing that.
> > > 
> > > Xen's CI from automation might need some updates to use latest
> > > stable
> > > versions:
> > > * Fedora 29 is EOL, should use at least Fedora 31
> > > * Debian Jessie is EOL. Stretch is present, but Buster is missing
> > 
> > We're working on the CI loop.
> > 
> > As maintainer, it is ultimately Christian's choice to as to if/when
> > to
> > bump the minimum versions.
> > 
> > 
> > As a general rule, we don't want to be sufficiently bleeding edge
> > to
> > rule out in-use distros.  I have no idea if 4.06 is ok there, or
> > whether
> > it is too new.  Then again, the Ocaml components are strictly
> > optional
> > so it is perhaps less important.
> > 
> > Whatever happens WRT version, the configure change should occur
> > before
> > changes in the code which would fail on older versions.
> 
> Yes I would like to see the bump happen before applying a version of
> this series too.

I have a branch that removes the requirement on 4.06 from the series,
and then have a separate one that bumps to 4.06 and removes the
redundant and inefficient workaround.
In the end I only had to backport 2 functions: Map.find_opt and
Map.update.

Will send out the updated series for review once I've done some more
testing on it.

Best regards,
--Edwin

> 
> Wei.
> 
> > ~Andrew