C implementation of the Bron Kerbosch #313
Conversation
james-d-mitchell
added
the
performance
There was a problem hiding this comment.
Some things to comment on:
- It'd be best if you can replace the current version of the clique finder in GAP with your version, so that we can ensure that backwards compatibility is maintained, and that the tests work.
- It'd be good to see some tests where the new code is faster than the old (and/or vice versa)
- run clang-format, cpplint, and valgrind on the code just to double check that all is ok.
src/cliques.c
Outdated
|
|
||
| if (TNUM_OBJ((Obj) user_param) == T_PLIST_EMPTY) { | ||
| RetypeBag(user_param, T_PLIST); | ||
| } |
There was a problem hiding this comment.
This shouldn't be necessary (and I would recommend avoiding it!) as you use ASS_LIST below (good!)
src/cliques.c
Outdated
| const uint16_t); | ||
| static void* USER_PARAM; // A USER_PARAM for the hook | ||
|
|
||
| static jmp_buf OUTOFHERE; // So we can jump out of the deepest |
There was a problem hiding this comment.
Just to remark: this code won't work in a multi threaded setting due to all these global variables :-(.
The usual solution I'd use would be to put these into a simple struct, and then pass around a pointer to that struct to all functions that need it. At the top level, a member of that struct can be instantiated on the stack.
This can actually even be beneficial in terms of performance, as on many architectures, accessing global variables is slower than accessing a plain pointer.
There was a problem hiding this comment.
Thanks for the comments @fingolfin, much of the kernel module of Digraphs is not thread-safe, an issue that I'm aware of, but haven't really seen any reason to change until now.
There was a problem hiding this comment.
In practice I also don't care that much about thread safety, it is just usually the quickest way to convince people of my true desire: to avoid global variables as much as possible. They tend to cause all kinds of design issues down the road, and often make it much harder to reason about code that uses them. But here at least they are static and restricted to one relatively small source file.
Anyway, all that is just my opinion, and obviously that shouldn't count for much here, as I am not a stakeholder in this project :-). I merely stumbled over this PR and left some well-meant quick comments. Mind you, overall this code looks perfectly fine
There was a problem hiding this comment.
Thanks @fingolfin, the comments are much appreciated!
src/cliques.c
Outdated
| HOOK(USER_PARAM, CLIQUE, nr); | ||
| *nr_found += 1; | ||
| if (*nr_found >= limit) { | ||
| longjmp(OUTOFHERE, 1); |
There was a problem hiding this comment.
longjmp & setjmp are tricky beasts. I'd recommend to avoid them if possible. And here it really is possible, and quite easy: just change BronKerbosch to return an int; e.g. 0 for success, 1 for failure (or the other way around, doesn't matter). Then instead of doing a longjmp here and below, just return FAILURE; change all other return; to return SUCCESS;. And finally, in the recursive calls, just do if (FAILURE == BronKerbosch(...)) return FAILURE;
src/cliques.c
Outdated
| "or fail, not %s,", | ||
| (Int) TNAM_OBJ(size_obj), | ||
| 0L); | ||
| } else if (IS_INTOBJ(size_obj) && INT_INTOBJ(size_obj) <= 0) { |
There was a problem hiding this comment.
Just use IS_POS_INTOBJ -- or even better, combined this check with the previous one, and say must be a positive small integer in the error message?
src/cliques.c
Outdated
| } | ||
| Obj gens = CALL_1ARGS(GeneratorsOfGroup, aut_grp_obj); | ||
| DIGRAPHS_ASSERT(IS_LIST(gens)); | ||
| DIGRAPHS_ASSERT(LEN_LIST(gens) > 0); |
There was a problem hiding this comment.
So I realize this is unlikely to ever matter in practice, but I wonder: do you really want to crash if gens is bad data? Why not ErrorQuit here like for everything else, too? Or for that matter, simply remove the assertions, the first call to LEN_LIST will trigger an Error of its own if gens is not a list.
|
I noticed while valgrinding this that you don't free the memory allocated by Once these are resolved this looks good to merge. |
|
I've pushed fixes for the assertion/memory leaks to flsmith:cliques. |
|
@flsmith thanks for the fixes! |
|
Merged, thanks @jjonusas and sorry for the delay! |
Translated the gap implementation of Bron Kerbosch to C.