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

libidset: public lib for numerically sorted, non-negative integer sets #1498

Merged
merged 3 commits into from May 2, 2018

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented May 1, 2018

As discussed in #1496, this adds a new library that contains a minimal subset of the libutil/nodeset class, cleaned up and reworked for public consumption.

@dongahn, you might give this a try in flux-framework/flux-sched#330, e.g.

#include <flux/idset.h>

struct idset *idset = idset_decode (s_in);
if (!idset)
    // handle error
char *s_out = idset_encode (idset, IDSET_FLAG_RANGE);
if (!s_out)
    // handle error
// do something with s_out, a range-compressed version of s_in
free (s_out);
idset_destroy (idset);
@dongahn

dongahn approved these changes May 1, 2018


/* idset - a set of unordered, non-negative integers */

/* Implemented as a wrapper around a Van Emde Boas trie.

This comment has been minimized.

@dongahn

dongahn May 1, 2018

Contributor

Very very minor nit. trie -> Tree.

This comment has been minimized.

@garlick

garlick May 2, 2018

Author Member

Oops I think you're right, not a trie.


#include <sys/types.h>

/* An idset is an unordered set of non-negative integers (0,1,2,3...).

This comment has been minimized.

@dongahn

dongahn May 1, 2018

Contributor

Is this really an unordered set or ordered set?

If the input is "3,2,4,5", the output will become "3,2,4-5" or still "2-5"?

This comment has been minimized.

@garlick

garlick May 2, 2018

Author Member

should be 2-5. Maybe "unordered" is the wrong way to describe it? The order of insertion has no bearing on the order of the set - it is always encoded in sorted order.

This comment has been minimized.

@dongahn

dongahn May 2, 2018

Contributor

Thanks @garlick.

Not a big deal, but I have a slight preference towards ordered set in that case.

In C++,

unordered_set doesn't keep sorted keys internally. So there isn't a good way to output sorted keys.

set is sorted and can output the keys sorted.

This comment has been minimized.

@garlick

garlick May 2, 2018

Author Member

Thanks! I went with "numerically sorted" instead of unordered (or ordered).

* 'flags' may include IDSET_FLAG_AUTOGROW.
* Returns idset on success, or NULL on failure with errno set.
*/
struct idset *idset_create (size_t slots, int flags);

This comment has been minimized.

@dongahn

dongahn May 1, 2018

Contributor

Just so that I understand, what are slots argument represent, the maximum number of items between delimiters to be supported?

This comment has been minimized.

@garlick

garlick May 2, 2018

Author Member

Think of the idset as a bitmap. This is the highest numbered id (plus one) that can be inserted (unless IDSET_FLAG_AUTOGROW is set). Perhaps that could be clearer.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented May 1, 2018

This is great! I will try this out on flux-framework/flux-sched#330 soon.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 2, 2018

Coverage Status

Coverage increased (+0.04%) to 79.083% when pulling fcb978a on garlick:libidset into f8eccc5 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 2, 2018

Codecov Report

Merging #1498 into master will increase coverage by 0.05%.
The diff coverage is 88.59%.

@@            Coverage Diff             @@
##           master    #1498      +/-   ##
==========================================
+ Coverage   78.74%   78.79%   +0.05%     
==========================================
  Files         163      164       +1     
  Lines       30557    30706     +149     
==========================================
+ Hits        24061    24195     +134     
- Misses       6496     6511      +15
Impacted Files Coverage Δ
src/common/libidset/idset.c 88.59% <88.59%> (ø)
src/common/libflux/content.c 86.66% <0%> (-3.34%) ⬇️
src/common/libflux/request.c 87.17% <0%> (-1.29%) ⬇️
src/broker/overlay.c 73.81% <0%> (-0.64%) ⬇️
src/common/libflux/future.c 88.78% <0%> (-0.47%) ⬇️
src/common/libflux/message.c 81.25% <0%> (ø) ⬆️
src/modules/kvs/kvs.c 66.03% <0%> (+0.16%) ⬆️
src/broker/module.c 84.07% <0%> (+0.27%) ⬆️
src/common/libutil/base64.c 95.77% <0%> (+0.7%) ⬆️
... and 1 more

@garlick garlick changed the title libidset: public lib for unordered, non-negative integer sets libidset: public lib for numerically sorted, non-negative integer sets May 2, 2018

@garlick garlick force-pushed the garlick:libidset branch from 513e65c to 056c816 May 2, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 2, 2018

I went ahead and squashed the incremental development here.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 2, 2018

One more squash needed, then I'd be happy to merge this since @dongahn approved.

@garlick garlick force-pushed the garlick:libidset branch from 129496b to 4f2d8ed May 2, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 2, 2018

Ok, thanks, I squashed that one.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 2, 2018

Not a blocker for sure, but I just happened to notice that codecov shows idset_grow isn't fully tested (I only note it in case that is unexpected.)

https://codecov.io/gh/flux-framework/flux-core/pull/1498/diff#D6-237

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 2, 2018

Oh yeah, I'll fix that.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented May 2, 2018

Since it is so close to be merged, I will hold off testing this on the sched PR until this becomes available at the master. Thanks @garlick and @grondo for the nice work!

libidset: add idset class
Implement public replacement for the nodeset class as
discussed in issue #180.  This is a scaled down subset
of nodeset at this time.

Install the library as libflux-idset.so, with a flux-idset.pc
file for pkgconfig.

@garlick garlick force-pushed the garlick:libidset branch from 4f2d8ed to e04730b May 2, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 2, 2018

Testing should be better now (hopefully), and I also fixed a couple of other problems I noticed - encode buffer chunk size was tiny, and when I increased it to a reasonable value, I found I had not initialized it with a NULL and was calling strcat() on it! Glad we paused here for another pass.

@garlick garlick force-pushed the garlick:libidset branch from e04730b to fcb978a May 2, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 2, 2018

Actually... please hold off merging this for a bit. I'd like a last minute chance to review whether I'm using signed vs unsigned ints in all the right places here.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented May 2, 2018

@garlick: Sure. I have one task I have to finish this morning and then this will be the next priority.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 2, 2018

Actually it can go ahead and be merged if it passes travis, since there are currently no "ids" exposed as signed or unsigned ints in the API. Any re-jiggering would be internal, so it can wait.

@grondo grondo merged commit 12a1880 into flux-framework:master May 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 79.083%
Details

@garlick garlick deleted the garlick:libidset branch May 2, 2018

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.