search

facebookarchive / libphenom

An eventing framework for building high performance and high scalability systems in C.
http://facebook.github.io/libphenom
Apache License 2.0
1.66k stars 362 forks source link

Upgrade ck to 0.4.1 #59

Closed brucespang closed 10 years ago

brucespang commented 10 years ago

I have a libphenom application that uses concurrency-kit extensively. It needs some things from ck that are not available in 0.3.0. This upgrades to ck-0.4.1 to make those things available.

There were some changes to libphenom that were necessary due to breaking api changes in ck (https://github.com/sbahra/ck/commit/3edb523da5f6c9dcea41879ceb6643dcc7bde305). I've updated job.c and job.h to use the new api.

The libphenom tests pass, and I haven't seen any issues in my application level testing.

Some questions:

  1. Does libphenom have a better way to update ck than copying in the code for the new tag?
  2. Is there a preferred way to benchmark these changes to make sure they're not regressions?
wez commented 10 years ago

See also https://github.com/facebook/libphenom/pull/46 in which we thought that it would make sense to unbundle CK to make this a bit easier, or at least, make these pull requests smaller and easier to review :-)

Do you have the cycles to do the unbundling? I think the ideal path forward here is:

  1. Put up an unbundling pull request that just removes the bundled CK library, and revises the configure script to locate CK via pkg-config. It should also check the version of CK to make sure it is compatible (sounds like < 0.3.3). I think this one is conceptually a simple diff, maybe just a bit fiddly (obtain the "older" CK, install it somewhere like /tmp/ck-0.3.X, then tweak configure to use that one)
  2. Put up a separate pull request that handles the upgrade in CK version and the ck_ring or other API changes

I'm on PTO for a bit so will be online sporadically, but I'm happy to assist with this when I get my hands on a keyboard; that probably means mostly reviewing things but depends on how badly I suffer coding withdrawal ;-)

wez commented 10 years ago

Regarding benchmarks, there's tests/bench/run-pipes.php that repeatedly runs some benchmarks with varying concurrency and records the data then produces some nice charts using R. You can use that with the before/after to see if there are obvious regressions.

wez commented 10 years ago

and thanks for looking at this! :)

brucespang commented 10 years ago

Sounds good, thanks for the feedback! I definitely have time to work on this.

I've create PR #60 to do the unbundling. I'll have another pull request soon with the api change.

brucespang commented 10 years ago

I've created PR #61 for the changes to libphenom's use of ck_ring. I'm going to close this pull request, since we're now tracking it in the other two.