[CLIPPER-224] Replace boost futures with folly futures #266

Corey-Zumar · 2017-08-08T04:57:03Z

Fixes #224

AmplabJenkins · 2017-08-08T05:00:14Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/604/
Test FAILed.

AmplabJenkins · 2017-08-12T01:50:11Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/605/
Test FAILed.

Corey-Zumar · 2017-08-12T01:59:14Z

cmake/FindBoost.cmake

@@ -1,3 +1,6 @@
+# Distributed under the OSI-approved BSD 3-Clause License.  See accompanying


This file was out of date and caused issues with Boost 1.64.0.

Sounds good. Did you just find a more recent version of the file?

AmplabJenkins · 2017-08-12T02:00:11Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/606/
Test FAILed.

dcrankshaw · 2017-08-14T19:59:28Z

Did you do any benchmarking before/after this code change? I'm curious if we see any performance difference?

Corey-Zumar · 2017-08-14T20:09:46Z

Yes! There's a 40% improvement in throughput for noop_bench.

AmplabJenkins · 2017-08-16T00:38:43Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/616/
Test FAILed.

dcrankshaw

Just a couple minor comments. This looks great. It looks like once you got the cmake and docker stuff working, folly was basically a drop-in replacement for boost.

dcrankshaw · 2017-08-16T00:18:28Z

CMakeLists.txt

@@ -20,6 +20,15 @@ set_property(TARGET boost PROPERTY
 set_property(TARGET boost PROPERTY
    INTERFACE_LINK_LIBRARIES ${Boost_LIBRARIES})

+# Include Folly as an imported target
+find_package(Folly 2017.07.31.00 REQUIRED)


Can we pin to a less specific version of Folly? For example, the current version of Folly in homebrew is already a 2017.08.14.00.

It turns out that this version pinning was completely ineffective because FindFolly.cmake doesn't set any version flags, nor does it appear that folly specifies an installation version in its include directory. For now, I've removed the version entirely.

dcrankshaw · 2017-08-16T00:18:51Z

CMakeLists.txt

+add_library(folly INTERFACE IMPORTED)
+set_property(TARGET folly PROPERTY
+    INTERFACE_INCLUDE_DIRECTORIES ${FOLLY_INCLUDE_DIR})
+set(FOLLY_LINK_FLAGS "-lglog")


What is this flag?

This flag specifies that glog should also be linked when linking Folly. Without this flag, I ran into a build error on OSX indicating that glog wasn't found.

dcrankshaw · 2017-08-16T00:27:11Z

cmake/FindBoost.cmake

@@ -1,3 +1,6 @@
+# Distributed under the OSI-approved BSD 3-Clause License.  See accompanying


Sounds good. Did you just find a more recent version of the file?

dcrankshaw · 2017-08-16T00:29:00Z

src/benchmarks/src/end_to_end_bench.cpp

@@ -4,12 +4,11 @@
 #include <random>
 #include <vector>

-#include <boost/thread.hpp>


Can you remove all reference to boost/thread? I just grepped it, and there are still a bunch in the source code:

./src/libclipper/include/clipper/containers.hpp:#include <boost/thread.hpp> ./src/libclipper/include/clipper/datatypes.hpp:#include <boost/thread.hpp> ./src/libclipper/include/clipper/query_processor.hpp:#include <boost/thread.hpp> ./src/libclipper/include/clipper/task_executor.hpp:#include <boost/thread.hpp> ./src/libclipper/include/clipper/threadpool.hpp:#include <boost/thread.hpp> ./src/libclipper/include/clipper/timers.hpp:#include <boost/thread.hpp> ./src/libclipper/include/clipper/util.hpp:#include "boost/thread.hpp" ./src/libclipper/src/containers.cpp:#include <boost/thread.hpp> ./src/libclipper/src/metrics.cpp:#include <boost/thread.hpp> ./src/libclipper/src/persistent_state.cpp:#include <boost/thread.hpp> ./src/libclipper/src/query_processor.cpp:#include <boost/thread.hpp> ./src/libclipper/src/query_processor.cpp:#include <boost/thread/executors/basic_thread_pool.hpp> ./src/libclipper/src/rpc_service.cpp:#include <boost/thread.hpp> ./src/libclipper/src/task_executor.cpp:#include <boost/thread.hpp> ./src/libclipper/src/timers.cpp:#include <boost/thread.hpp> ./src/management/src/management_frontend.hpp:#include <boost/thread.hpp> ./src/management/src/management_frontend_tests.cpp:#include <boost/thread.hpp>

dcrankshaw · 2017-08-16T00:40:45Z

src/libclipper/src/query_processor.cpp

-  boost::promise<Response> response_promise;
-  auto response_future = response_promise.get_future();
+  // TODO(czumar): Find a more readable name for std::vector<folly::Unit> and
+  // typedef accordingly


I don't think this TODO is necessary. It's not a particularly complicated type and I'd rather leave it explicit.

Agreed, removed the TODO

dcrankshaw · 2017-08-16T00:59:24Z

src/libclipper/src/query_processor.cpp

+  std::shared_ptr<std::vector<Output>> outputs_ptr =
+      std::make_shared<std::vector<Output>>(std::move(outputs));
+
+  std::vector<folly::Future<folly::Unit>> wrapped_task_futures;


Are you wrapping the futures for error-handling purposes? Just want to make sure I understand what's going on.

I'm wrapping for error handling as well as aggregating future outputs in the vector owned by the shared pointer outputs_ptr. This aggregation is necessary because, in the case that the timer future completes before all of the task futures have completed, we need to access to the values of the completed tasks. folly::collectAny doesn't support this behavior directly, hence the wrapping.

Corey-Zumar

@dcrankshaw Addressed your comments

Corey-Zumar · 2017-08-16T08:28:36Z

src/libclipper/src/query_processor.cpp

-  boost::promise<Response> response_promise;
-  auto response_future = response_promise.get_future();
+  // TODO(czumar): Find a more readable name for std::vector<folly::Unit> and
+  // typedef accordingly


Agreed, removed the TODO

Corey-Zumar · 2017-08-16T08:33:46Z

src/libclipper/src/query_processor.cpp

+  std::shared_ptr<std::vector<Output>> outputs_ptr =
+      std::make_shared<std::vector<Output>>(std::move(outputs));
+
+  std::vector<folly::Future<folly::Unit>> wrapped_task_futures;


I'm wrapping for error handling as well as aggregating future outputs in the vector owned by the shared pointer outputs_ptr. This aggregation is necessary because, in the case that the timer future completes before all of the task futures have completed, we need to access to the values of the completed tasks. folly::collectAny doesn't support this behavior directly, hence the wrapping.

Corey-Zumar · 2017-08-16T08:49:00Z

CMakeLists.txt

@@ -20,6 +20,15 @@ set_property(TARGET boost PROPERTY
 set_property(TARGET boost PROPERTY
    INTERFACE_LINK_LIBRARIES ${Boost_LIBRARIES})

+# Include Folly as an imported target
+find_package(Folly 2017.07.31.00 REQUIRED)


It turns out that this version pinning was completely ineffective because FindFolly.cmake doesn't set any version flags, nor does it appear that folly specifies an installation version in its include directory. For now, I've removed the version entirely.

Corey-Zumar · 2017-08-16T08:49:34Z

src/benchmarks/src/end_to_end_bench.cpp

@@ -4,12 +4,11 @@
 #include <random>
 #include <vector>

-#include <boost/thread.hpp>


AmplabJenkins · 2017-08-25T01:17:45Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/628/
Test FAILed.

into folly_futures

AmplabJenkins · 2017-08-25T01:47:38Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/629/
Test FAILed.

AmplabJenkins · 2017-08-25T02:07:26Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/630/
Test FAILed.

Corey-Zumar · 2017-08-25T02:18:07Z

jenkins test this please

AmplabJenkins · 2017-08-25T02:54:21Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/631/
Test FAILed.

AmplabJenkins · 2017-08-25T03:17:48Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/632/
Test FAILed.

This reverts commit 5604672.

AmplabJenkins · 2017-08-25T04:01:15Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/633/
Test PASSed.

dcrankshaw

LGTM. Just need to give the ARG CODE_VERSION in the dockerfiles a default value.

dcrankshaw · 2017-08-25T05:59:47Z

ManagementFrontendDockerfile

@@ -1,4 +1,5 @@
-FROM frolvlad/alpine-gxx
+ARG CODE_VERSION


Give this arg a default version (eg https://github.com/ucbrise/clipper/blob/develop/PythonContainerDockerfile#L1), otherwise the automated builds on Docker Hub will fail.

dcrankshaw · 2017-08-25T06:00:05Z

QueryFrontendDockerfile

@@ -1,9 +1,11 @@
-FROM frolvlad/alpine-gxx
+ARG CODE_VERSION


Give this a default argument (see above comment)

Corey-Zumar · 2017-08-25T06:17:18Z

@dcrankshaw Added default versions to management frontend and query frontend docker files.

AmplabJenkins · 2017-08-25T07:12:02Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/634/
Test FAILed.

Corey-Zumar · 2017-08-25T07:37:24Z

jenkins test this please

AmplabJenkins · 2017-08-25T07:58:27Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/635/
Test FAILed.

dcrankshaw · 2017-08-25T16:14:47Z

jenkins test this please

AmplabJenkins · 2017-08-25T17:06:51Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/636/
Test PASSed.

* rpc test infra change * rpcbench more * bench, metrics fix * concurrent queue for rpc * Add send/recv balance to rpc bench * multi container bench * add concurrent queue * fix end_to_end bench * [CLIPPER-224] Replace boost futures with folly futures (ucbrise#266) * replace boost futures with folly * Remove unused futures code * some code formatting * Update query frontend dockerfile to support Folly * fix cache move issue * remove unused import * format code * remove version check * remove boost thread dependency * remove boost thread as required component * format code * Create base dockerfile for installing clipper deps, query and mgmt inherit from this base * rename lib-base >> lib_base * makes FindFolly.cmake search in FOLLY_ROOT * format code * Pin library versions in base dockerfile * Fix FindFolly cmake * Add additional link flags for folly cmake * Add missing threadpool imports * fix boost thread import * Add more cmake link flags * Revert "Add more cmake link flags" This reverts commit 5604672. * Update QueryFrontendDockerfile * Update ManagementFrontendDockerfile * query proc return default future * grpc benchmark script * Add request throughput meter to grpc frontend * change meter name * Add metrics reporting * update grpc bench * fixes * frontend fixes * update grpc benchmark * multiproc changes * more bench fixes * allow query proc to flourish * logging perf * batch size log * Management frontend: Improve clarity of response messages (ucbrise#271) * format code * Update clipper mgr register external output * logs * queue size * Fix histogram overflow errors (ucbrise#272) * Recalculate mean on insertion, improve histogram data precision and value capacity * Format code * Implement CLOCK policy * add cache tests * revert clipper mgr changes * remove quotes * add test assertion * format code * Refactor PredictionCache to evict based on byte size rather than num pages * Add config flag for cache size * Format code * Update config tests * format code * Update config.hpp * Remove unused task executor threadpool size config * Add cache size option to clipper mgr * format code * remove task execution variable * when evicting, stop if we're out of pages * remove task execution test statement * Fix rebase issue * updates * fix * qp limit * fix * fix test * qp lat * fix * fox * test * test2 * test3 * test4 * test5 * Wangle * more wangle * threadpool * .. * link wangle * bench log * update * .. * test * test * testfix * test2 * fixes * var naming * more metrics * time diff * .. * one more metric change * queue insert metric * more seg monitor * measure something that probably isn't the issue * measure something that probably is the problem * fixes * hopeful move fix * measure other part of cache * measure cache put * measure with some wangle * wangle with the frontend * more measure * hone measurement * try fix * var issues * add move * make copy * Add a log * expect tid match * remove log * another continuation change * bump hist size * change cache seg to fetch * urgh * hm * fix segfault * var naming * fix merge issues * measure cache insertion latency * cache eviction * cache unique ptr * fix method prototype * fixes * remove hit ratio * remove lookups counter * bench other seg * metric move * change qp seg hist * move hist * .. * hist size * ... * send/recv balance * meterfix * remove metrics * experiment * varfix * .. * ... * .. * logging * varfix * why * measure elsewhere * measure thread task lat * compfixes * compfix2 * compfix3 * fail to fix seg fault * fix segfault * get num steps * time first pass * track queue insertion * log output * Add moar ctx * Remove log * .. * Add a logging tag * varfix * zmq frontend * cmake fix * import fixes * remove grpc stuff * fixes * more fixes * fixes again * fix * zmq client * varfox * add socket bind * string conversion * update * .. * .... * test * ... * protocol fix * fix? * fail * fix polling * client fix * api fix * fixes * actually add the application to rpc * debug * bench fix * client logs * client log * new log * Add hash checks * remove a log * log * test * routing id log * cmake fix * import fix * client fix * routing id fix * .. * constness * try again * prototype changes * constness * constness * client test * client test * more routing id logs' * remove ref * remove move * routing id rpc log * ... * req id int * ordering * fix really dumb error * prototype fixes * client throughput logging * client fix * client fix2 * units * .. * .. * clean up task executor * cleanup qp * cleanup * bench update * remove unnecessary file handles from grpc bench * remove unused chron statements * re-add deadline comparator

Corey-Zumar added 3 commits August 7, 2017 19:37

replace boost futures with folly

9edb5a7

Remove unused futures code

fb14f1b

some code formatting

31a87dd

Corey-Zumar added status: in progress type: enhancement labels Aug 8, 2017

Update query frontend dockerfile to support Folly

e90ad47

Corey-Zumar added 2 commits August 11, 2017 18:52

fix cache move issue

632a837

remove unused import

e19fe59

Corey-Zumar commented Aug 12, 2017

View reviewed changes

Corey-Zumar requested a review from dcrankshaw August 12, 2017 01:59

Corey-Zumar added status: needs review and removed status: in progress labels Aug 12, 2017

format code

1679bf7

dcrankshaw requested changes Aug 16, 2017

View reviewed changes

dcrankshaw added status: needs revision and removed status: needs review labels Aug 16, 2017

Corey-Zumar added 5 commits August 16, 2017 01:24

remove version check

3380693

remove boost thread dependency

e51daa3

remove boost thread as required component

efca36e

merge

1f86054

format code

c5ea6b8

Corey-Zumar commented Aug 16, 2017

View reviewed changes

Corey-Zumar added status: needs review and removed status: needs revision labels Aug 16, 2017

Corey-Zumar added 2 commits August 24, 2017 18:35

Add missing threadpool imports

0a44998

Merge branch 'folly_futures' of https://github.com/Corey-Zumar/clipper-1

3107dc5

into folly_futures

fix boost thread import

b20532a

Add more cmake link flags

5604672

Revert "Add more cmake link flags"

7463722

This reverts commit 5604672.

dcrankshaw requested changes Aug 25, 2017

View reviewed changes

dcrankshaw added status: needs revision and removed status: needs review labels Aug 25, 2017

Corey-Zumar added 2 commits August 24, 2017 23:16

Update QueryFrontendDockerfile

66a96da

Update ManagementFrontendDockerfile

d320012

Corey-Zumar added status: needs review and removed status: needs revision labels Aug 25, 2017

dcrankshaw added status: accepted and removed status: needs review labels Aug 25, 2017

dcrankshaw approved these changes Aug 25, 2017

View reviewed changes

dcrankshaw merged commit 1429039 into ucbrise:develop Aug 25, 2017

		@@ -1,3 +1,6 @@
		# Distributed under the OSI-approved BSD 3-Clause License. See accompanying

[CLIPPER-224] Replace boost futures with folly futures #266

[CLIPPER-224] Replace boost futures with folly futures #266

Conversation

Corey-Zumar commented Aug 8, 2017 • edited Loading

AmplabJenkins commented Aug 8, 2017

AmplabJenkins commented Aug 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Aug 12, 2017

dcrankshaw commented Aug 14, 2017

Corey-Zumar commented Aug 14, 2017

AmplabJenkins commented Aug 16, 2017

dcrankshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Corey-Zumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Aug 25, 2017

AmplabJenkins commented Aug 25, 2017

AmplabJenkins commented Aug 25, 2017

Corey-Zumar commented Aug 25, 2017

AmplabJenkins commented Aug 25, 2017

AmplabJenkins commented Aug 25, 2017

AmplabJenkins commented Aug 25, 2017

dcrankshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Corey-Zumar commented Aug 25, 2017

AmplabJenkins commented Aug 25, 2017

Corey-Zumar commented Aug 25, 2017

AmplabJenkins commented Aug 25, 2017

dcrankshaw commented Aug 25, 2017

AmplabJenkins commented Aug 25, 2017

Corey-Zumar commented Aug 8, 2017 •

edited

Loading