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.

Sign up for GitHub

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

remove allocations in hostrange_find #6259

Merged
merged 2 commits into from
Sep 11, 2024
Merged

remove allocations in hostrange_find #6259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26d24c7
libhostlist: increase performance of hostlist_find()
trws Aug 6, 2024
92f327a
libhostlist: add unit tests for hostlist_find_hostname()
grondo Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 41 additions & 11 deletions src/common/libhostlist/hostlist.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include "hostlist.h"
#include "hostrange.h"
#include "hostname.h"
#include "util.h"

/* number of elements to allocate when extending the hostlist array */
Expand Down Expand Up @@ -384,7 +385,7 @@ static int hostlist_append_host (struct hostlist *hl, const char *str)
{
int rc = -1;
struct hostrange * hr;
struct hostname * hn;
struct hostlist_hostname * hn;

assert (hl != NULL);

Expand Down Expand Up @@ -687,16 +688,10 @@ int hostlist_count (struct hostlist *hl)
}

static int hostlist_find_host (struct hostlist *hl,
const char *hostname,
struct stack_hostname *hn,
struct current *cur)
{
int i, count, ret = -1;
struct hostname * hn;

hn = hostname_create (hostname);
if (!hn)
return -1;

for (i = 0, count = 0; i < hl->nranges; i++) {
int offset = hostrange_hn_within (hl->hr[i], hn);
if (offset >= 0) {
Expand All @@ -708,7 +703,6 @@ static int hostlist_find_host (struct hostlist *hl,
count += hostrange_count (hl->hr[i]);
}

hostname_destroy (hn);
if (ret < 0)
errno = ENOENT;
return ret;
Expand All @@ -721,7 +715,38 @@ int hostlist_find (struct hostlist *hl, const char *hostname)
errno = EINVAL;
return -1;
}
return hostlist_find_host (hl, hostname, &hl->current);
struct stack_hostname hn_storage;

struct stack_hostname *hn = hostname_stack_create (&hn_storage, hostname);
if (!hn)
return -1;
return hostlist_find_host (hl, hn, &hl->current);
}

int hostlist_find_hostname (struct hostlist *hl, struct hostlist_hostname *hn)
{
struct stack_hostname hn_storage;
struct stack_hostname *shn;

if (!hl || !hn) {
errno = EINVAL;
return -1;
}

shn = hostname_stack_create_from_hostname (&hn_storage, hn);
if (!hn)
return -1;
return hostlist_find_host (hl, shn, &hl->current);
}

struct hostlist_hostname *hostlist_hostname_create (const char *hn)
{
return hostname_create (hn);
}

void hostlist_hostname_destroy (struct hostlist_hostname *hn)
{
hostname_destroy (hn);
}

/* Remove host at cursor 'cur'. If the current real cursor hl->current
Expand Down Expand Up @@ -786,7 +811,12 @@ static int hostlist_remove_at (struct hostlist *hl, struct current *cur)
static int hostlist_delete_host (struct hostlist *hl, const char *hostname)
{
struct current cur = { 0 };
int n = hostlist_find_host (hl, hostname, &cur);
struct stack_hostname hn_storage;

struct stack_hostname *hn = hostname_stack_create (&hn_storage, hostname);
if (!hn)
return -1;
int n = hostlist_find_host (hl, hn, &cur);
if (n < 0)
return errno == ENOENT ? 0 : -1;
return hostlist_remove_at (hl, &cur);
Expand Down
29 changes: 29 additions & 0 deletions src/common/libhostlist/hostlist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,35 @@ const char * hostlist_nth (struct hostlist * hl, int n);
*/
int hostlist_find (struct hostlist * hl, const char *hostname);

/*
* Create an opaque hostname object which can be used with the
* more efficient hostlist_find_hostname() below. This interface
* should be used when matching the same hostname to many hostlists
* since it caches processing of the hostname instead of duplicating
* the work on each call to hostlist_find().
*
* Caller must call hostlist_hostname_destroy() to free memory associated
* with the returns hostlist_hostname object.
*
* Returns NULL on error.
*/
struct hostlist_hostname *hostlist_hostname_create (const char *hostname);

/*
* Free resources associated with hostname hn.
*/
void hostlist_hostname_destroy (struct hostlist_hostname *hn);

/*
* Search hostlist hl for the first host matching hostlist_hostname `hn`
* and return position in list if found.
*
* Leaves cursor pointing to the matching host.
*
* Returns -1 if host is not found.
*/
int hostlist_find_hostname (struct hostlist *hl, struct hostlist_hostname *hn);

/*
* Delete all hosts in the list represented by `hosts'
*
Expand Down
137 changes: 121 additions & 16 deletions src/common/libhostlist/hostname.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
#include <string.h>
#include <ctype.h>
#include <errno.h>
#include <math.h>
#include <stdio.h>
#include <string.h>

#include "hostname.h"

Expand All @@ -25,9 +27,10 @@
/*
* return the location of the last char in the hostname prefix
*/
static int host_prefix_end (const char *hostname)
static int host_prefix_end (const char *hostname, int len)
{
int len = strlen (hostname);
if (len < 0)
return -1;
int idx = len - 1;

/* Rewind to the last non-digit in hostname
Expand All @@ -44,16 +47,101 @@
static int hostname_len (const char *hostname)
{
int len = strlen (hostname);
for (int i = 0; i < len; i++)
if (strchr (INVALID_CHARS, hostname[i]))
return -1;
if (len != strcspn (hostname, INVALID_CHARS))
return -1;
return len;
}

struct hostname * hostname_create_with_suffix (const char *hostname,
int idx)
struct stack_hostname *hostname_stack_create_from_hostname (
struct stack_hostname *hn,
struct hostlist_hostname *src)
{
if (!hn || !src) {
errno = EINVAL;
return NULL;

Check warning on line 61 in src/common/libhostlist/hostname.c

View check run for this annotation

Codecov / codecov/patch

src/common/libhostlist/hostname.c#L60-L61 

Added lines #L60 - L61 were not covered by tests
}
hn->hostname = src->hostname;
hn->suffix = src->suffix;
hn->len = src->len;
hn->len_prefix = src->len_prefix;
hn->width = src->width;
hn->num = src->num;
return hn;
}

struct stack_hostname *hostname_stack_create_with_suffix (
struct stack_hostname *hn,
const char *hostname,
int len,
int idx)
{
struct hostname * hn = NULL;
if (!hostname || !hn || len < 0) {
Fixed Show fixed Hide fixed
errno = EINVAL;
return NULL;

Check warning on line 80 in src/common/libhostlist/hostname.c

View check run for this annotation

Codecov / codecov/patch

src/common/libhostlist/hostname.c#L79-L80 

Added lines #L79 - L80 were not covered by tests
}
hn->hostname = hostname;
hn->len = len;
hn->len_prefix = idx + 1;
hn->num = 0;
hn->suffix = NULL;
if (idx == hn->len - 1) {
return hn;
}
hn->suffix = hn->hostname + hn->len_prefix;

char *p = NULL;
hn->num = strtoul (hn->suffix, &p, 10);
if (p == hn->suffix && hn->num == 0) {
errno = EINVAL;
return NULL;

Check warning on line 96 in src/common/libhostlist/hostname.c

View check run for this annotation

Codecov / codecov/patch

src/common/libhostlist/hostname.c#L95-L96 

Added lines #L95 - L96 were not covered by tests
}
hn->width = hn->len - hn->len_prefix;
return hn;
}

struct stack_hostname *hostname_stack_copy_one_less_digit (
struct stack_hostname *dst,
struct stack_hostname *src)
{

if (!dst || !src) {
errno = EINVAL;
return NULL;

Check warning on line 109 in src/common/libhostlist/hostname.c

View check run for this annotation

Codecov / codecov/patch

src/common/libhostlist/hostname.c#L108-L109 

Added lines #L108 - L109 were not covered by tests
}
*dst = *src;
dst->len_prefix = src->len_prefix + 1;
if (src->len_prefix == dst->len - 1) {
return dst;
}
dst->suffix = dst->hostname + dst->len_prefix;

dst->width = dst->len - dst->len_prefix;
if (dst->width < 0 || dst->width > 10) {
errno = EINVAL;
return NULL;

Check warning on line 121 in src/common/libhostlist/hostname.c

View check run for this annotation

Codecov / codecov/patch

src/common/libhostlist/hostname.c#L120-L121 

Added lines #L120 - L121 were not covered by tests
}

// remove the most significant decimal digit without reparsing
// lookup table because pow is slow
static const int pow10[10] = {
1,
10,
100,
1000,
10000,
100000,
1000000,
10000000,
100000000,
1000000000};
dst->num = src->num % pow10[dst->width];
return dst;
}

struct hostlist_hostname *hostname_create_with_suffix (const char *hostname,
int idx)
{
struct hostlist_hostname *hn = NULL;
int len;
char *p = NULL;

Expand All @@ -71,10 +159,13 @@
}

hn->num = 0;
hn->len = len;
hn->len_prefix = idx + 1;
hn->width = hn->len - hn->len_prefix;
hn->prefix = NULL;
hn->suffix = NULL;

if (idx == strlen (hostname) - 1) {
if (idx == len - 1) {
if ((hn->prefix = strdup (hostname)) == NULL) {
hostname_destroy (hn);
return NULL;
Expand All @@ -101,23 +192,37 @@
return hn;
}

struct stack_hostname *hostname_stack_create (struct stack_hostname *hn,
const char *hostname)
{
int len = 0;
if (!hostname || (len = hostname_len (hostname)) < 0) {
errno = EINVAL;
return NULL;
}
return hostname_stack_create_with_suffix (hn,
hostname,
len,
host_prefix_end (hostname, len));
}

/*
* create a struct hostname * object from a string hostname
* create a struct hostlist_hostname * object from a string hostname
*/
struct hostname * hostname_create (const char *hostname)
struct hostlist_hostname *hostname_create (const char *hostname)
{
int end;
if (!hostname) {
errno = EINVAL;
return NULL;
}
return hostname_create_with_suffix (hostname,
host_prefix_end (hostname));
end = host_prefix_end (hostname, hostname_len (hostname));
return hostname_create_with_suffix (hostname, end);
}

/* free a hostname object
*/
void hostname_destroy (struct hostname * hn)
void hostname_destroy (struct hostlist_hostname * hn)
{
int saved_errno = errno;
if (hn) {
Expand All @@ -131,14 +236,14 @@

/* return true if the hostname has a valid numeric suffix
*/
int hostname_suffix_is_valid (struct hostname * hn)
int hostname_suffix_is_valid (struct hostlist_hostname * hn)
{
return (hn && hn->suffix != NULL);
}

/* return the width (in characters) of the numeric part of the hostname
*/
int hostname_suffix_width (struct hostname * hn)
int hostname_suffix_width (struct hostlist_hostname * hn)
{
if (!hn) {
errno = EINVAL;
Expand Down
40 changes: 34 additions & 6 deletions src/common/libhostlist/hostname.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,49 @@
* Convenience structure representing a hostname as a prefix,
* optional numeric part, and suffix.
*/
struct hostname {
struct hostlist_hostname {
char *hostname; /* cache of initialized hostname */
char *prefix; /* hostname prefix */
int len; /* length minus invalid characters */
int len_prefix; /* length of the prefix */
int width; /* length of the suffix */
unsigned long num; /* numeric suffix */

/* string representation of numeric suffix
* points into `hostname' */
char *suffix;
};

struct hostname * hostname_create (const char *s);
struct hostname * hostname_create_with_suffix (const char *s, int i);
void hostname_destroy (struct hostname *hn);
struct stack_hostname {
const char *hostname; /* cache of initialized hostname */
int len; /* length minus invalid characters */
int len_prefix; /* length of the prefix */
int width; /* length of the suffix */
unsigned long num; /* numeric suffix */

/* string representation of numeric suffix
* points into `hostname' */
const char *suffix;
};

struct hostlist_hostname * hostname_create (const char *s);
struct hostlist_hostname * hostname_create_with_suffix (const char *s, int i);
struct stack_hostname *hostname_stack_create (struct stack_hostname *hn,
const char *hostname);
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);
Comment on lines +47 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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. 🤞

void hostname_destroy (struct hostlist_hostname *hn);

int hostname_suffix_is_valid (struct hostname *hn);
int hostname_suffix_width (struct hostname *hn);
int hostname_suffix_is_valid (struct hostlist_hostname *hn);
int hostname_suffix_width (struct hostlist_hostname *hn);

#endif /* !HAVE_FLUX_HOSTLIST_HOSTNAME_H */
Loading
Loading