libczmqcontainers: introduce new internal library of czmq `zhashx` library #3596

chu11 · 2021-04-15T05:47:15Z

Per discussion in #3591, bring in a new internal copy of zhashx into flux-core to deal with CZMQ issue zeromq/czmq#2173.

to avoid symbol collisions, renamed zhashx to fzhashx in the local copy
to avoid search and replace everywhere, add zhashx_int.h which does pre-processor macros to change zhashx to fzhashx everywhere.

@garlick mentioned we may want to do tweaks later as well, to perhaps not assert on malloc errors, etc. But I will leave those fixes for another day. Perhaps we will want to create an issue to remember.

lgtm-com · 2021-04-15T06:45:19Z

This pull request introduces 1 alert when merging eb7c4bb into e2034e2 - view on LGTM.com

new alerts:

1 for FIXME comment

garlick · 2021-04-15T15:01:42Z

Thanks for this and for the upstream work!

One problem with renaming everything in the copied czmq source is that it's hard to diff later. Could we instead leave the source mostly pristine and just include the header with the rename defines?

It might also be clearer to someone down the line if we left the source file names as they were, which I don't think will affect anything.

Finally, would it be possible to avoid all the include additions to our source by using something like AH_BOTTOM to add our local include to config.h and then have the ability to turn it off in one place later?

garlick · 2021-04-15T15:20:19Z

using something like AH_BOTTOM to add our local include to config.h

Er, I'm not sure how great of an idea that would be since it kind of hides everything. It might be better to just change the name of the new include to something more generic that could include multiple container classes, like src/common/libczmqcontainers/containers.h? Then if we decided to drop one and go back to using the installed czmq version, we could just drop it in there.

grondo · 2021-04-15T15:22:19Z

One problem with renaming everything in the copied czmq source is that it's hard to diff later. Could we instead leave the source mostly pristine and just include the header with the rename defines?

This is what I had been thinking when I suggested using preprocessor to redefine symbol names.

At first I was thinking of modifying just the czmq header files to include the preprocessor defines. You could presumably add -I src/common/libczmqcontainers to our includes path to force our local copy to be included first. (Now that I see @garlick's suggestion of a containers.h, I like that approach better though)

garlick · 2021-04-15T15:32:11Z

Another thought would be to add stuff we need to detach the classes from czmq to a separate include like standalone.h (for example the freen() macro). There might be more of those over time for different classes.

Then instead of defining stuff directly in zhashx.c, it could just include standalone.h (bonus: reducing the size of the diff).

chu11 · 2021-04-16T13:36:40Z

@garlick and @grondo, some good ideas! I like the idea of the standalone.h to move the misc things I need to add to keep the diff small (like freen()). Then containers.hcan have all of our symbol renamings and be included by all internal callers.

chu11 · 2021-04-16T19:26:37Z

re-pushed. Now we have these files:

src/common/libczmqcontainers/czmq_containers.h
src/common/libczmqcontainers/czmq_internal.h
src/common/libczmqcontainers/fzhashx.c
src/common/libczmqcontainers/fzhashx.h
src/common/libczmqcontainers/zhashx_map.h

with callers to load czmq_containers.h.

The diff is pretty tiny now with it mostly being my comments on what the file is.

14,26c14,15
< /* This is a copy of the czmq zhashx library.
<  *
<  * Due to czmq issue #2173
<  * (https://github.com/zeromq/czmq/issues/2173) performance in
<  * flux-core was extremely poor at times and flux-core could not
<  * wait for the OS distros to pick up the bug fix.
<  *
<  * This file is a verbatim copy with only minor adjustments to header
<  * guards and included headers.
<  */
< 
< #ifndef __FZHASHX_H_INCLUDED__
< #define __FZHASHX_H_INCLUDED__
---
> #ifndef __ZHASHX_H_INCLUDED__
> #define __ZHASHX_H_INCLUDED__
32,34d20
< #include <czmq.h>
< #include "czmq_internal.h"
< #include "zhashx_map.h"
286a273
>

>diff src/common/libczmqcontainers/fzhashx.c ~achu/code/czmq/src/zhashx.c 
14,24d13
< /* This is a copy of the czmq zhashx library.
<  *
<  * Due to czmq issue #2173
<  * (https://github.com/zeromq/czmq/issues/2173) performance in
<  * flux-core was extremely poor at times and flux-core could not
<  * wait for the OS distros to pick up the bug fix.
<  *
<  * This file is a verbatim copy with only minor adjustments to
<  * included headers.
<  */
< 
38c27
< #include "fzhashx.h"
---
> #include "czmq_classes.h"
48c37
< #include "fzhash_primes.inc"
---
> #include "zhash_primes.inc"

lgtm-com · 2021-04-16T19:55:37Z

This pull request introduces 1 alert when merging 00a39ad into 7aaea06 - view on LGTM.com

new alerts:

1 for FIXME comment

chu11 · 2021-04-16T19:59:51Z

As an side, the LGTM failure is b/c there is a "FIXME" in the zhashx implementation. So it was copied in. Won't be fixing that one so the code/comments will stay the same.

garlick · 2021-04-16T20:34:40Z

Nice!

Any reason not to just leave the source files zhashx.c, zhashx.h, zhash_primes.h?

The multi-include protection in zhashx.h would still need to be different of course.

Our commit that adds the copy of the czmq source should probably reference the foreign commit and a czmq release so we can quickly check if we're behind. Might also be good to include a note on the czmq license and a reference to the MPL in (L)GPL discussion you referenced earlier.

Problem: A bug in the czmq zhashx library would incorrectly resize the hash. See: zeromq/czmq#2173 This would lead to significant performance issues as the hash would take up far more memory than it should and iteration of the hash would take an excess amount of time. A solution to this problem was fixed in: zeromq/czmq#2174 however the fix will not exist in most OS distributions for some time. Solution: We have copied in the zhashx implementation into a new convenience library libczmqcontainers. Code was copied from the master branch at commit d4b1a1d884532e43e82b9055ceb0b84d1db5915c. The library is copied in verbatim with only minor changes to headers and header guards. To avoid excess copying in from czmq, add a file czmq_internal.h that copies in macros, headers, and typdefs needed by the localized zhashx. To avoid symbol collisions, add a file zhashx_map.h that will convert all internal uses of "zhashx" to "fzhashx". Add a generic czmq_containers.h, that callers can include to use the internal zhashx over the shared library version. All files containing copied in code maintain their czmq license (Mozilla Public License Version 2.0). All modifications to czmq files and code were isolated to those files, no code from other parts of flux-core were copied in. See: https://www.mozilla.org/en-US/MPL/2.0/combining-mpl-and-gpl/

Problem: To avoid bugs in most OS distro's versions of czmq, we want internal flux callers to use the internal "fzhashx" library instead of the czmq "zhashx" library whenever a "zhashx" is used. Solution: Include the file "czmq_containers.h" whereever zhashx data structures are used. Update Makefile.am files to add libczmqcontainers.la when appropriate. Fixes flux-framework#3591

codecov · 2021-04-16T23:00:12Z

Codecov Report

Merging #3596 (70d11b7) into master (7aaea06) will decrease coverage by 0.49%.
The diff coverage is 42.08%.

❗ Current head 70d11b7 differs from pull request most recent head 6771b05. Consider uploading reports for the commit 6771b05 to get more accurate results

@@            Coverage Diff             @@
##           master    #3596      +/-   ##
==========================================
- Coverage   82.56%   82.07%   -0.50%     
==========================================
  Files         324      325       +1     
  Lines       48888    49482     +594     
==========================================
+ Hits        40366    40611     +245     
- Misses       8522     8871     +349

Impacted Files	Coverage Δ
src/broker/content-cache.c	`70.83% <ø> (ø)`
src/broker/runat.c	`84.58% <ø> (ø)`
src/cmd/flux-exec.c	`73.12% <ø> (ø)`
src/common/libflux/attr.c	`93.33% <ø> (ø)`
src/common/libflux/msg_handler.c	`90.40% <ø> (ø)`
src/common/libjob/job_hash.c	`87.50% <ø> (ø)`
src/common/libpmi/simple_server.c	`88.55% <ø> (ø)`
src/common/librlist/rlist.c	`88.61% <ø> (ø)`
src/common/librlist/rnode.c	`90.57% <ø> (ø)`
src/common/librouter/disconnect.c	`85.91% <ø> (ø)`
... and 35 more

chu11 · 2021-04-16T23:00:15Z

re-pushed with tweaks per @garlick comment above

lgtm-com · 2021-04-16T23:14:40Z

This pull request introduces 1 alert when merging 6771b05 into 7aaea06 - view on LGTM.com

new alerts:

1 for FIXME comment

garlick

This looks good to me! Thanks!

chu11 force-pushed the issue3591_internal_zhashx branch from adcde20 to eb7c4bb Compare April 15, 2021 06:30

chu11 force-pushed the issue3591_internal_zhashx branch from eb7c4bb to 1dac3f0 Compare April 16, 2021 19:22

chu11 force-pushed the issue3591_internal_zhashx branch 2 times, most recently from 6473bdc to 00a39ad Compare April 16, 2021 19:41

chu11 added 2 commits April 16, 2021 15:27

chu11 force-pushed the issue3591_internal_zhashx branch from 00a39ad to 6771b05 Compare April 16, 2021 23:00

garlick approved these changes Apr 17, 2021

View reviewed changes

chu11 added the merge-when-passing label Apr 17, 2021

mergify bot merged commit fd99b1b into flux-framework:master Apr 17, 2021

chu11 mentioned this pull request Apr 17, 2021

libczmqcontainers: introduce new internal library of czmq zhashx library flux-framework/flux-sched#822

Merged

This was referenced Apr 24, 2021

throughput of jobs slows down over time #3583

Closed

libczmqcontainers: include zlistx, zhash, zlist, and convert internal users #3620

Merged

chu11 deleted the issue3591_internal_zhashx branch June 5, 2021 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libczmqcontainers: introduce new internal library of czmq `zhashx` library #3596

libczmqcontainers: introduce new internal library of czmq `zhashx` library #3596

chu11 commented Apr 15, 2021

lgtm-com bot commented Apr 15, 2021

garlick commented Apr 15, 2021

garlick commented Apr 15, 2021

grondo commented Apr 15, 2021

garlick commented Apr 15, 2021

chu11 commented Apr 16, 2021

chu11 commented Apr 16, 2021

lgtm-com bot commented Apr 16, 2021

chu11 commented Apr 16, 2021

garlick commented Apr 16, 2021

codecov bot commented Apr 16, 2021

chu11 commented Apr 16, 2021

lgtm-com bot commented Apr 16, 2021

garlick left a comment

libczmqcontainers: introduce new internal library of czmq zhashx library #3596

libczmqcontainers: introduce new internal library of czmq zhashx library #3596

Conversation

chu11 commented Apr 15, 2021

lgtm-com bot commented Apr 15, 2021

garlick commented Apr 15, 2021

garlick commented Apr 15, 2021

grondo commented Apr 15, 2021

garlick commented Apr 15, 2021

chu11 commented Apr 16, 2021

chu11 commented Apr 16, 2021

lgtm-com bot commented Apr 16, 2021

chu11 commented Apr 16, 2021

garlick commented Apr 16, 2021

codecov bot commented Apr 16, 2021

Codecov Report

chu11 commented Apr 16, 2021

lgtm-com bot commented Apr 16, 2021

garlick left a comment

Choose a reason for hiding this comment

libczmqcontainers: introduce new internal library of czmq `zhashx` library #3596

libczmqcontainers: introduce new internal library of czmq `zhashx` library #3596