Skip to content

Conversation

@psafont
Copy link
Member

@psafont psafont commented Dec 12, 2025

The Listext module has a lot of baggage that can be replace with Stdlib, Astring. And what cannot be replaced, it can be made better, especially the escaping.

There are quite a few changes, so it's better to review commit-by-commit.

I need to do some testing to make sure all changes here are safe and undraft this

Users can instead opt for Astring.String's or stdlib's functions.

Signed-off-by: Pau Ruiz Safont <[email protected]>
Signed-off-by: Pau Ruiz Safont <[email protected]>
Also remove all indiscriminate opens against it

Signed-off-by: Pau Ruiz Safont <[email protected]>
Signed-off-by: Pau Ruiz Safont <[email protected]>
Signed-off-by: Pau Ruiz Safont <[email protected]>
Signed-off-by: Pau Ruiz Safont <[email protected]>
@cplaursen
Copy link
Contributor

I'll run some system tests on this and have a proper look throughout the day.

@lindig
Copy link
Contributor

lindig commented Dec 15, 2025

Has String.replaced been benchmarked?

psafont and others added 5 commits December 15, 2025 11:32
String.replaced is an alias of map_unlikely. This names makes the intent
of the function clearer. Because a function to replace the characters is
exposed, users are less likely to fall into the pitfall of using lists.

Lists not only are very slow, but allow users to have more than one
replacement rule per character, possibly introducing mistakes.

If a plain match function cannot be produced and a list needs to be used,
users can convert it to a Char.Map and do the match with a find_opt.
This approach ends up being ~60-70% faster than using plain lists.

The benchmark comparing the new approach with the old one:

  String size 100:
    Optimized: 236.556 μs
    Reference: 1861.600 μs
    Improvement: 87.3% faster

  String size 500:
    Optimized: 1099.030 μs
    Reference: 9665.405 μs
    Improvement: 88.6% faster

  String size 1000:
    Optimized: 2198.777 μs
    Reference: 19115.019 μs
    Improvement: 88.5% faster

Signed-off-by: Pau Ruiz Safont <[email protected]>
Not only it's more efficient, but it's also more ergonomic

Signed-off-by: Pau Ruiz Safont <[email protected]>
Signed-off-by: Pau Ruiz Safont <[email protected]>
The former didn't have any tests and the performance is unknown

Signed-off-by: Pau Ruiz Safont <[email protected]>
Signed-off-by: Pau Ruiz Safont <[email protected]>
This also allows to drop String.isspace

Signed-off-by: Pau Ruiz Safont <[email protected]>
Signed-off-by: Pau Ruiz Safont <[email protected]>
The few users that needed to replace strings, have been replaced with Astring's
cuts, as most of them were already segmenting strings, or they are run in very
specific, infrequent codepaths for efficiency to not matter.

Others have been replaced by Astring's filter as they were removing characters,
and the rest have been converted to the new String.replace.

map_unlikely can be removed from the interface and only have String.replaced
and String.replace

Signed-off-by: Pau Ruiz Safont <[email protected]>
Signed-off-by: Pau Ruiz Safont <[email protected]>
@psafont
Copy link
Member Author

psafont commented Dec 15, 2025

Yes, from the message of the commit that introduces it (1174f3a):

If a plain match function cannot be produced and a list needs to be used,
users can convert it to a Char.Map and do the match with a find_opt.
This approach ends up being ~60-70% faster than using plain lists.

The benchmark comparing the new approach with the old one:

  String size 100:
    Optimized: 236.556 μs
    Reference: 1861.600 μs
    Improvement: 87.3% faster

  String size 500:
    Optimized: 1099.030 μs
    Reference: 9665.405 μs
    Improvement: 88.6% faster

  String size 1000:
    Optimized: 2198.777 μs
    Reference: 19115.019 μs
    Improvement: 88.5% faster

It can also be easily run with dune exec ocaml/libs/xapi-stdext/lib/xapi-stdext-std/bench/bench_xstringext.exe

@cplaursen
Copy link
Contributor

So I ran a test suite on the changes and unfortunately a few of the tests failed. Not sure what the best way to share the test results are, but here are some failure excerpts:

XenLivePatchTest:

Expecting RecommendedGuidance is {'RestartToolstack'}, but found RecommendedGuidance is {'RestartToolstack', 'reboot_host_on_xen_livepatch_failure'}
Expecting RecommendedGuidance is set(), but found RecommendedGuidance is {'reboot_host_on_xen_livepatch_failure'}

Networking:

15/12/2025 10:58:17	Host reboot of !genuk-21-05d timed out
15/12/2025 10:58:17	Error running get_console_logs: SSH connection refused (/opt/xenrt/scripts/get_console_logs /tmp/local/scratch/xenrt/guest-console-logs 1765793412 None)
15/12/2025 10:58:17	Retrying CLI command host-get-system-status-capabilities -s 10.71.240.94 -u root -pw '************' uuid=2d825f4f-2967-4126-8327-87ea11742ea6
15/12/2025 10:58:17	Retrying CLI command host-get-system-status-capabilities -s 10.71.240.94 -u root -pw '************' uuid=2d825f4f-2967-4126-8327-87ea11742ea6
15/12/2025 10:58:18	Retrying CLI command host-get-system-status-capabilities -s 10.71.240.94 -u root -pw '************' uuid=2d825f4f-2967-4126-8327-87ea11742ea6
15/12/2025 10:58:18	Exception from host-get-system-status-capabilities: Host genuk-21-05d is not pingable on 10.71.240.94

HAOCx2WE:

15/12/2025 12:14:10	Error running get_console_logs: SSH connection refused (/opt/xenrt/scripts/get_console_logs /tmp/local/scratch/xenrt/guest-console-logs 1765794201 None)
15/12/2025 12:14:12	Exception from host-get-system-status-capabilities: CLI command host-get-system-status-capabilities failed: You attempted an operation which involves a host which could not be contacted.
15/12/2025 12:14:14	Poking Xen from job 4518430 to give us a crashdump on genus-35-071d

Let me know how you'd like to proceed/what info you need from me.

@psafont
Copy link
Member Author

psafont commented Dec 15, 2025

It's difficult to say whether they are related just by looking at those loglines, and I haven't seen the test runs in months to remember well how they could affect the update guidances, or hosts failing to get online.

Thanks for making the builds and running the tests, it's good to know most tests passed! (I guess the ring3BVT+BST)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants