Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code
On Thu, 2020-08-27 at 09:41 +, 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; d...@recoil.org; w...@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
Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code
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; d...@recoil.org; w...@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
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; d...@recoil.org; w...@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
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; d...@recoil.org; w...@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
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
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
Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code
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
[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
