* [PATCH v1 0/6] tools/ocaml/xenstored: simplify code
@ 2020-08-14 22:11 Edwin Török
2020-08-17 10:12 ` Christian Lindig
2020-08-17 12:56 ` Christian Lindig
0 siblings, 2 replies; 8+ messages in thread
From: Edwin Török @ 2020-08-14 22:11 UTC (permalink / raw)
To: xen-devel
Cc: Edwin Török, Christian Lindig, David Scott,
Ian Jackson, Wei Liu
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code 2020-08-14 22:11 [PATCH v1 0/6] tools/ocaml/xenstored: simplify code Edwin Török @ 2020-08-17 10:12 ` Christian Lindig 2020-08-17 12:56 ` Christian Lindig 1 sibling, 0 replies; 8+ messages in thread From: Christian Lindig @ 2020-08-17 10:12 UTC (permalink / raw) To: Edwin Torok, xen-devel; +Cc: David Scott, Ian Jackson, Wei Liu 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code 2020-08-14 22:11 [PATCH v1 0/6] tools/ocaml/xenstored: simplify code Edwin Török 2020-08-17 10:12 ` Christian Lindig @ 2020-08-17 12:56 ` Christian Lindig 2020-08-18 7:28 ` Edwin Torok 1 sibling, 1 reply; 8+ messages in thread From: Christian Lindig @ 2020-08-17 12:56 UTC (permalink / raw) To: Edwin Torok, xen-devel; +Cc: David Scott, Ian Jackson, Wei Liu ________________________________________ 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code 2020-08-17 12:56 ` Christian Lindig @ 2020-08-18 7:28 ` Edwin Torok 2020-08-18 9:25 ` Christian Lindig 0 siblings, 1 reply; 8+ messages in thread From: Edwin Torok @ 2020-08-18 7:28 UTC (permalink / raw) To: Christian Lindig, xen-devel; +Cc: Ian Jackson, dave, wl 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code 2020-08-18 7:28 ` Edwin Torok @ 2020-08-18 9:25 ` Christian Lindig 2020-08-18 12:40 ` Andrew Cooper 0 siblings, 1 reply; 8+ messages in thread From: Christian Lindig @ 2020-08-18 9:25 UTC (permalink / raw) To: Edwin Torok, xen-devel; +Cc: Ian Jackson, dave, wl 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code 2020-08-18 9:25 ` Christian Lindig @ 2020-08-18 12:40 ` Andrew Cooper 2020-08-27 9:41 ` Wei Liu 0 siblings, 1 reply; 8+ messages in thread From: Andrew Cooper @ 2020-08-18 12:40 UTC (permalink / raw) To: Christian Lindig, Edwin Torok, xen-devel; +Cc: Ian Jackson, dave, wl 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code 2020-08-18 12:40 ` Andrew Cooper @ 2020-08-27 9:41 ` Wei Liu 2020-08-27 10:11 ` Edwin Torok 0 siblings, 1 reply; 8+ messages in thread From: Wei Liu @ 2020-08-27 9:41 UTC (permalink / raw) To: Andrew Cooper Cc: Christian Lindig, Edwin Torok, xen-devel, Ian Jackson, dave, wl 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 0/6] tools/ocaml/xenstored: simplify code 2020-08-27 9:41 ` Wei Liu @ 2020-08-27 10:11 ` Edwin Torok 0 siblings, 0 replies; 8+ messages in thread From: Edwin Torok @ 2020-08-27 10:11 UTC (permalink / raw) To: wl, Andrew Cooper; +Cc: Ian Jackson, dave, Christian Lindig, xen-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-27 10:11 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-14 22:11 [PATCH v1 0/6] tools/ocaml/xenstored: simplify code Edwin Török 2020-08-17 10:12 ` Christian Lindig 2020-08-17 12:56 ` Christian Lindig 2020-08-18 7:28 ` Edwin Torok 2020-08-18 9:25 ` Christian Lindig 2020-08-18 12:40 ` Andrew Cooper 2020-08-27 9:41 ` Wei Liu 2020-08-27 10:11 ` Edwin Torok
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).