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

planner: Replace zhash_t to zhashx_t for higher performance #391

Merged
merged 1 commit into from Sep 29, 2018

Conversation

Projects
None yet
4 participants
@dongahn
Copy link
Contributor

dongahn commented Sep 29, 2018

planner_new () can be called from a hot section
of the code and performance profilers discovered that
zhash_new () is way too slow.

With zhash_t, planner_new () accouted for 9.1% of the
graph data store populating. With zhashx_t, it
only accouts for 2.88% for the same operation.

See Issue #390.

planner: Replace zhash_t to zhashx_t for higher performance
planner_new () can be called from a hot section
of the code and performance profilers discovered that
zhash_new () is way too slow.

With zhash_t, planner_new () accouted for 9.1% of the
graph data store populating. With zhashx_t, it
only accouts for 9.1% for the same operation.

@dongahn dongahn requested a review from SteVwonder Sep 29, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 29, 2018

Codecov Report

Merging #391 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #391   +/-   ##
======================================
  Coverage      76%     76%           
======================================
  Files          64      64           
  Lines       10175   10175           
======================================
  Hits         7734    7734           
  Misses       2441    2441
Impacted Files Coverage Δ
resource/planner/planner.c 78.89% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c27dc14...ad63124. Read the comment docs.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 29, 2018

Coverage Status

Coverage remained the same at 77.745% when pulling ad63124 on dongahn:zhash_perf_fix into c27dc14 on flux-framework:master.

@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Sep 29, 2018

Thanks @dongahn! LGTM. Any idea how much faster zlistx is than zlist?

@SteVwonder SteVwonder merged commit e67f932 into flux-framework:master Sep 29, 2018

4 checks passed

codecov/patch 100% of diff hit (target 76%)
Details
codecov/project 76% (+0%) compared to c27dc14
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 77.745%
Details
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.