remove allocations in hostrange_find #6259

trws · 2024-09-04T23:22:12Z

This is an in-progress PR looking at making hostrange_find allocation-free. It actually does that, and also keeps track of the length of all string components such that it can use comparison functions that don't have to check for a null terminator, but when I was doing tests with this a few weeks ago, it was no faster for the flux-sched case. Maybe worth poking at to see if I did something screwy but something strange is going on.

src/common/libhostlist/hostname.c

trws · 2024-09-05T18:17:09Z

I decided to look back in on this and try it in a microbenchmark today just to see if maybe it was an interaction with sched that made it look like no improvement. After a few more tweaks, I'm getting about a 3x performance improvement for this test:

#include <benchmark/benchmark.h>
#include "../src/common/libhostlist/hostlist.h"

static void BM_SomeFunction(benchmark::State& state) {
  hostlist *hl = hostlist_decode("host1[1,2-50]");
  // Perform setup here
  for (auto _ : state) {
    // This code gets timed
    if (hostlist_find(hl, "host151") >= 0)
      exit (1);
    if (hostlist_find(hl, "host149") < 0)
      exit (2);
  }
}
// Register the function as a benchmark
BENCHMARK(BM_SomeFunction);

For the sched case, I think it would be an even bigger improvement if I had a way to create a "hostname" handle that sched could keep hold of. In core we could kinda reach into hostlist and do that now, but sched relies on the external stable interface. How would you feel about exposing something for a "hostname" sched could hold on to? It could be opaque just like the hostlist, but not having to re-scan the thing every time would be a massive saving, even if it takes an allocation.

src/common/libhostlist/hostname.c

grondo · 2024-09-05T19:59:51Z

How would you feel about exposing something for a "hostname" sched could hold on to?

That sounds reasonable for a performant search or match. What would you think the interface would be, something like:

struct hostlist_hostname *hostlist_hostname_create (const char *, size_t len);

int hostlist_find_hostname (struct hostlist *hl, struct hostlist_hostname *hostname);

?

trws · 2024-09-05T20:17:14Z

That's what I was originally thinking yeah, I'm playing with whether I could get away with re-using the existing hostlist and have a hostlist_find_current(struct hostlist *hl, struct hostlist *hl_src) that searches for the hostname pointed to by the cursor of hl_src. Not sure if that will turn out, but might help keep the interface smaller.

trws · 2024-09-05T20:32:14Z

Nope, not practical, the hostname interface like you posed above is much more feasible because of the way hostrange is laid out. The main thing to keep it cheap is not to have to parse the int, re-validate, and re-strlen the thing.

grondo · 2024-09-05T20:46:01Z

Yeah, the only other idea I had was a more generic struct hostlist_finder or hostlist_matcher interface, because that could be more extensible in the future, but honestly I can't think of what else you'd use it for ATM.

trws · 2024-09-05T22:56:39Z

This probably needs some stylistic tweaks, but it's working and I need to drop it for a bit to take care of some admin stuff. Part of me really wants to get rid of having two separate structs for the borrowed, or "stack", hostname but getting rid of the "prefix" member as a null-terminated string that's just the prefix is definitely a big rework.

grondo · 2024-09-06T14:25:33Z

This looks good to me besides some cleanup that is probably just a result of this being a WIP. If it is helpful I can do the stylistic changes (mainly just formatting) and add some tests for the new interface.

trws · 2024-09-06T20:35:50Z

Yeah the formatting is definitely messed up in a few places, the main thing I want to make sure gets done (other than tests) is renaming the struct from hostname to hostlist_hostname so we don't accidentally squat on something so generic externally. Let me see if I can get to it in the next couple of hours, if not the start of next week is crazy enough that I'd greatly appreciate the help. I also want to check this version with sched, if it works out the way my microbenchmark did, the sched use-case should be ~10x faster with all of this put together.

grondo · 2024-09-06T20:47:00Z

Ok, I'll wait for updates and plan to work on this Monday if you don't get to it first. thanks @trws!

trws · 2024-09-06T22:30:22Z

Ok, I think I got it cleaned up. Happy with this if you are @grondo.

trws · 2024-09-06T22:56:38Z

sigh helps if I actually push all the files, should go much better this time.

grondo · 2024-09-06T22:57:32Z

Some trivial cleanup still needed (just formatting afaics, and sorry we don't have a valid clang format for our style 😞)

Also, something weird is happening in the asan build. It still seems to be picking up a struct hostname from somewhere:

  make  test_util.t test_hostname.t test_hostrange.t test_hostlist.t
    CC       test/hostname_t-hostname.o
  test/hostname.c: In function ‘main’:
  test/hostname.c:51:31: error: initialization of ‘struct hostname *’ from incompatible pointer type ‘struct hostlist_hostname *’ [-Werror=incompatible-pointer-types]
     51 |         struct hostname *hn = hostname_create (t->input);
        |                               ^~~~~~~~~~~~~~~

Edit: oh, this is probably related to your comment above, nvm!

trws · 2024-09-06T23:05:45Z

Yeah, I still need to get the updated clang-format file merged and set to only apply where we've already applied it. It's definitely possible I missed some things though, I'm not good at catching them (part of why I always try to automate that).

grondo · 2024-09-09T20:50:52Z

Going through this one again and I think probably the easiest thing to do here would be to squash all these commits together into one commit like libhostlist: improve hostlist_find performance by reducing allocations or similar. Then we still need to add a commit that adds unit tests for the new hostlist_hostname based functions.

Also, there's still some style issues.

Finally, I'd suggest renaming either the struct stack_hostname or the constructors for this struct hostname_stack_create() so they match.

@trws, I can take care of that if you are busy (and don't mind me rebasing and force-pushing to your branch)

trws · 2024-09-09T20:55:01Z

I am fine with all of that. Honestly I was hoping I could merge the stack hostname out and just have one struct, but it isn’t feasible without a lot more work. If you would I’d appreciate it, or I can but it will probably have to wait until at least Wednesday. Would actually like to know what format things I missed, they aren’t jumping out at me and that means something is missing or wrong in my mental model. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Mark Grondona ***@***.***> Sent: Monday, September 9, 2024 1:51:14 PM To: flux-framework/flux-core ***@***.***> Cc: Scogland, Tom ***@***.***>; Mention ***@***.***> Subject: Re: [flux-framework/flux-core] remove allocations in hostrange_find (PR #6259) Going through this one again and I think probably the easiest thing to do here would be to squash all these commits together into one commit like libhostlist: improve hostlist_find performance by reducing allocations or similar. Then we still need to add a commit that adds unit tests for the new hostlist_hostname based functions. Also, there's still some style issues. Finally, I'd suggest renaming either the struct stack_hostname or the constructors for this struct hostname_stack_create() so they match. @trws<https://urldefense.us/v3/__https://github.com/trws__;!!G2kpM7uM-TzIFchu!02WQvxuGgT_Vs-Khes2rWk1xvFSM8opWslCmviSD-prIYsJ-6XP3dk3mj4Vte4lWjNj_LYM1Ce-krkv9ZR4ieZjEk8o$>, I can take care of that if you are busy (and don't mind me rebasing and force-pushing to your branch) — Reply to this email directly, view it on GitHub<https://urldefense.us/v3/__https://github.com/flux-framework/flux-core/pull/6259*issuecomment-2339065089__;Iw!!G2kpM7uM-TzIFchu!02WQvxuGgT_Vs-Khes2rWk1xvFSM8opWslCmviSD-prIYsJ-6XP3dk3mj4Vte4lWjNj_LYM1Ce-krkv9ZR4ilvlAzdI$>, or unsubscribe<https://urldefense.us/v3/__https://github.com/notifications/unsubscribe-auth/AAFBFNI4NNN4HQ2EFATB6ILZVYC4FAVCNFSM6AAAAABNVJQJW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZZGA3DKMBYHE__;!!G2kpM7uM-TzIFchu!02WQvxuGgT_Vs-Khes2rWk1xvFSM8opWslCmviSD-prIYsJ-6XP3dk3mj4Vte4lWjNj_LYM1Ce-krkv9ZR4irsmD5Ck$>. You are receiving this because you were mentioned.Message ID: ***@***.***>

grondo · 2024-09-09T21:47:56Z

Would actually like to know what format things I missed, they aren’t jumping out at me and that means something is missing or wrong in my mental model.

Actually looking back I don't believe you really missed anything related to formatting. I saw some parameters broken onto the next line with one indent, but I think that was because you were just breaking a long line, which seems fine to me.

grondo · 2024-09-10T03:23:28Z

Ok @trws - pushed directly here an update that:

squashes the 3 previous commits into one
fixes the comment description of hostlist_hostname_create()
adds unit tests for hostlist_find_hostname()

trws · 2024-09-10T23:56:53Z

Looks good to me, anything further you'd like to see here?

grondo · 2024-09-11T00:04:32Z

Nothing more from me. Thanks! I can approve since all I made were some comment and very minor whitespace changes, and added unit tests.

grondo

LGTM! Thanks

grondo · 2024-09-06T22:44:41Z

src/common/libhostlist/hostname.h

+struct stack_hostname *hostname_stack_create_from_hostname (
+    struct stack_hostname *hn,
+    struct hostlist_hostname *hn_src);
+struct stack_hostname *hostname_stack_create_with_suffix (
+    struct stack_hostname *hn,
+    const char *hostname,
+    int len,
+    int idx);
+struct stack_hostname *hostname_stack_copy_one_less_digit (
+    struct stack_hostname *dst,
+    struct stack_hostname *src);


nit: formatting doesn't match current flux-core coding style, first parameter on long list goes on same line as function, the rest of the parameters are one per line, aligned with opening parens. (And so sorry we don't have a good clang format for this)

Does that override the line length? If I join these to the open peren then they stretch to as far as 85 cols. Hence the break in this case, this is actually what clang-format does when told to do what flux core style wants but given too little space. We discussed increasing to ~88 as the break point in the clang-format PR but had never officially agreed to do it.

These were old comments I didn't know GitHub was going to post. Sorry! I think your assessment and the formatting is correct here

No worries, I guessed that was it after I read the next couple. I'm going to clean up the clang-format file update PR in a bit, have a bit of time I can use for that this morning. Also have a much better solution to making sure it's consistent now than I used to. Hope jury duty isn't too bad btw. 🤞

grondo · 2024-09-06T22:46:00Z

src/common/libhostlist/hostname.c

+struct stack_hostname * hostname_stack_create_with_suffix (struct stack_hostname *hn,
+        const char *hostname, unsigned long len,  int idx) {


nit: style, break long parameter lists one per line. Opening function brace should go on its own line.

Oof, that one's awful. I thought I had disabled this, thanks for catching it.

grondo · 2024-09-09T20:44:53Z

src/common/libhostlist/hostlist.h

+/*
+ *  Search hostlist hl for the first host matching hostname
+ *   and return position in list if found.
+ *
+ *  Leaves cursor pointing to the matching host.
+ *
+ *  Returns -1 if host is not found.
+ */
+struct hostlist_hostname *hostlist_hostname_create (const char *);


Documentation for this function is coped from hostlist_find() and needs to be updated.

grondo · 2024-09-09T20:45:54Z

src/common/libhostlist/hostname.c

-    static const int pow10[10] = {
-        1, 10, 100, 1000, 10000,
-        100000, 1000000, 10000000, 100000000, 1000000000
-    };
+    static const int pow10[10] = {1,
+                                  10,
+                                  100,
+                                  1000,
+                                  10000,
+                                  100000,
+                                  1000000,
+                                  10000000,
+                                  100000000,
+                                  1000000000};


reformat probably doesn't belong in this commit.

problem: hostrange_hn_within requires an allocated hostname object, which must be re-created for every query. This is very expensive when the queries are frequent or repeated, especially in hostlist_find_host solution: add a new function hostrange_str_within that uses the exact same logic without building a newly allocated hostname object Use the existing string lengths in more places, and instead of re-parsing the suffix find the power of 10 matching the length of the string, and mod by that to chop off the extra digit in the recursive case Finally, add an interface to do all the prep work and return an opaque handle to the hostname object hostlist uses internally so a caller can re-use a pre-built object with the new hostlist_find_hostname interface. The result comes out a bit over twice as fast as directly using the allocation-free interface.

Problem: There are no tests of hostlist_find_hostname(). Add them using the existing hostlist_find() inputs.

codecov · 2024-12-10T01:00:22Z

Codecov Report

Attention: Patch coverage is 90.99099% with 10 lines in your changes missing coverage. Please review.

Project coverage is 83.35%. Comparing base (a3785a3) to head (92f327a).
Report is 505 commits behind head on master.

Files with missing lines	Patch %	Lines
src/common/libhostlist/hostname.c	85.07%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6259      +/-   ##
==========================================
- Coverage   83.35%   83.35%   -0.01%     
==========================================
  Files         521      521              
  Lines       85723    85793      +70     
==========================================
+ Hits        71453    71509      +56     
- Misses      14270    14284      +14

Files with missing lines	Coverage Δ
src/common/libhostlist/hostlist.c	`94.49% <100.00%> (+0.35%)`	⬆️
src/common/libhostlist/hostrange.c	`96.42% <100.00%> (+0.02%)`	⬆️
src/common/libhostlist/hostname.c	`82.75% <85.07%> (-1.12%)`	⬇️

... and 12 files with indirect coverage changes

trws requested a review from grondo September 4, 2024 23:22

trws marked this pull request as draft September 4, 2024 23:22

github-advanced-security bot found potential problems Sep 4, 2024

View reviewed changes

src/common/libhostlist/hostname.c Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Sep 5, 2024

View reviewed changes

src/common/libhostlist/hostname.c Fixed Show fixed Hide fixed

trws force-pushed the hostrange-find branch from 56497ff to e817278 Compare September 5, 2024 19:50

trws force-pushed the hostrange-find branch from 5639cda to abd7373 Compare September 5, 2024 23:37

trws marked this pull request as ready for review September 5, 2024 23:49

trws force-pushed the hostrange-find branch 2 times, most recently from 87a1943 to 9204fb2 Compare September 6, 2024 22:28

trws force-pushed the hostrange-find branch from 9204fb2 to 6c1f769 Compare September 6, 2024 22:56

grondo force-pushed the hostrange-find branch 2 times, most recently from 6fc817c to b229fcc Compare September 10, 2024 03:21

grondo approved these changes Sep 11, 2024

View reviewed changes

trws added the merge-when-passing label Sep 11, 2024

trws and others added 2 commits September 11, 2024 00:24

libhostlist: add unit tests for hostlist_find_hostname()

92f327a

Problem: There are no tests of hostlist_find_hostname(). Add them using the existing hostlist_find() inputs.

trws force-pushed the hostrange-find branch from b229fcc to 92f327a Compare September 11, 2024 00:24

mergify bot merged commit 3405555 into flux-framework:master Sep 11, 2024
33 checks passed

grondo mentioned this pull request Sep 18, 2024

hostlist: perf issue in hostlist_find_host() due to hostname_create() #5987

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove allocations in hostrange_find #6259

remove allocations in hostrange_find #6259

trws commented Sep 4, 2024

trws commented Sep 5, 2024

grondo commented Sep 5, 2024 •

edited

Loading

trws commented Sep 5, 2024

trws commented Sep 5, 2024

grondo commented Sep 5, 2024

trws commented Sep 5, 2024

grondo commented Sep 6, 2024

trws commented Sep 6, 2024

grondo commented Sep 6, 2024

trws commented Sep 6, 2024

trws commented Sep 6, 2024

grondo commented Sep 6, 2024 •

edited

Loading

trws commented Sep 6, 2024

grondo commented Sep 9, 2024

trws commented Sep 9, 2024 via email

grondo commented Sep 9, 2024

grondo commented Sep 10, 2024

trws commented Sep 10, 2024

grondo commented Sep 11, 2024

grondo left a comment

grondo Sep 6, 2024

trws Sep 11, 2024

grondo Sep 11, 2024

trws Sep 11, 2024

grondo Sep 6, 2024

trws Sep 11, 2024

grondo Sep 9, 2024

grondo Sep 9, 2024

codecov bot commented Dec 10, 2024

		struct stack_hostname * hostname_stack_create_with_suffix (struct stack_hostname *hn,
		const char *hostname, unsigned long len, int idx) {

remove allocations in hostrange_find #6259

remove allocations in hostrange_find #6259

Conversation

trws commented Sep 4, 2024

trws commented Sep 5, 2024

grondo commented Sep 5, 2024 • edited Loading

trws commented Sep 5, 2024

trws commented Sep 5, 2024

grondo commented Sep 5, 2024

trws commented Sep 5, 2024

grondo commented Sep 6, 2024

trws commented Sep 6, 2024

grondo commented Sep 6, 2024

trws commented Sep 6, 2024

trws commented Sep 6, 2024

grondo commented Sep 6, 2024 • edited Loading

trws commented Sep 6, 2024

grondo commented Sep 9, 2024

trws commented Sep 9, 2024 via email

grondo commented Sep 9, 2024

grondo commented Sep 10, 2024

trws commented Sep 10, 2024

grondo commented Sep 11, 2024

grondo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 10, 2024

Codecov Report

grondo commented Sep 5, 2024 •

edited

Loading

grondo commented Sep 6, 2024 •

edited

Loading