Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CP-52225: Misc improvements #3

Merged

Conversation

last-genius
Copy link
Contributor

@last-genius last-genius commented Nov 12, 2024

CP-52225 - poll: Reduce unnecessary allocations

No need to maintain interface compatibility with select anymore,
allocate an array of connection's poll status and modify that instead of
allocating an array, three lists, and a hash table on each iteration of
the main listening loop.


poll_select optimizations remove 10% of samples from the entire xenstore loop during a stress-test at creating 512 VMs:

xenstore-old-poll

xenstore-new-poll

@last-genius last-genius force-pushed the private/asultanov/poll_improvement branch from a2a64d6 to 172fcad Compare November 12, 2024 15:15
@last-genius
Copy link
Contributor Author

Added another cleanup commit

@psafont
Copy link
Member

psafont commented Nov 13, 2024

Are there any limitations to the minimum supported version of OCaml imposed by the Xen project? Otherwise I'm happy to support 4.14 as the minimum

Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring higher versions of OCaml is acceptable

@last-genius last-genius marked this pull request as draft December 2, 2024 13:53
@last-genius
Copy link
Contributor Author

Figured out some XenRT issues and re-testing this

@lindig
Copy link
Contributor

lindig commented Dec 3, 2024

This is draft but has green ticks - merge it?

@last-genius
Copy link
Contributor Author

no, I've discovered issues while testing and am figuring out a fix

@last-genius last-genius marked this pull request as ready for review December 3, 2024 14:47
@last-genius
Copy link
Contributor Author

last-genius commented Dec 3, 2024

Please review the fixup commit - I've missed spec_fds in the original commit:

-      try Poll.poll_select (spec_fds @ inset) outset [] timeout
+      try Poll.poll_select cons.poll_status timeout

so this required some workarounds. This is currently running BST+BVT and looks good (and confirmed manually to work).

@last-genius
Copy link
Contributor Author

Passed Ring3 BST+BVT.

@last-genius last-genius force-pushed the private/asultanov/poll_improvement branch from af9b41a to 9e792c4 Compare December 4, 2024 11:55
@last-genius last-genius force-pushed the private/asultanov/poll_improvement branch 2 times, most recently from 86ff9e1 to 23f618b Compare December 12, 2024 08:29
@last-genius
Copy link
Contributor Author

Fixed the merge conflict.

@edwintorok could you please let me know if there's anything blocking on your side or if the remaining comments are just suggestions? I haven't seen the discussed functions take up a significant amount of samples in flamegraphs so didn't undertake any further microoptimizations here.

Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily a blocker, but there are 2 small improvements that could be made: drop an unneeded let _, and replace the mutable spec_fds with parameter passing (avoid changing spec_fds from immutable to mutable).

@last-genius last-genius force-pushed the private/asultanov/poll_improvement branch 2 times, most recently from 2cc1805 to 7ecc609 Compare December 12, 2024 10:05
@last-genius
Copy link
Contributor Author

Testing the latest changes - looks like something is broken....

@last-genius last-genius force-pushed the private/asultanov/poll_improvement branch from 7ecc609 to 2cc1805 Compare December 12, 2024 15:24
@last-genius
Copy link
Contributor Author

Spent most of the day debugging the issues last optimizations (not allocating a new event every time) introduced to no avail, so I'm reverting them back. Current version looks good in BST+BVT

last-genius and others added 3 commits December 13, 2024 10:20
No need to maintain interface compatibility with select anymore,
allocate an array of connection's poll status and modify that instead of
allocating an array, three lists, and a hash table on each iteration of
the main listening loop.

Signed-off-by: Andrii Sultanov <[email protected]>
String.fold_left and String.starts_with are available since OCaml 4.13

Signed-off-by: Andrii Sultanov <[email protected]>
@last-genius last-genius force-pushed the private/asultanov/poll_improvement branch from 2cc1805 to 7064d59 Compare December 13, 2024 10:21
@last-genius
Copy link
Contributor Author

BST+BVT passed, squashed the fixup, merging.

@last-genius last-genius merged commit c8e8ac3 into xapi-project:main Dec 13, 2024
2 checks passed
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.

5 participants