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

Start of benchmark suite #11

Merged
merged 2 commits into from
Jan 29, 2015
Merged

Start of benchmark suite #11

merged 2 commits into from
Jan 29, 2015

Conversation

mroth
Copy link
Contributor

@mroth mroth commented Jan 25, 2015

As part of the work I've been doing in mroth/twitter-streaming-showdown, I've been looking at the performance and features of various Twitter streaming libraries. While ExTwitter is miles ahead of most in terms of having clear and well tested code, it's relative performance in high-throughput environments is less optimal.

Looking through the stream code, I have some ideas as to how to possibly make it more efficient I'd like to experiment with. But of course, you can't improve performance if you can't measure it!

Thus, as a first step, I wanted to create a benchmark test around stream processing so I could test these changes in a controlled fashion.

However, I'm having some difficulty getting the benchmarks to work so I'm doing an early PR to start a conversation. Details to follow in comment.

@mroth
Copy link
Contributor Author

mroth commented Jan 25, 2015

@parroty, I was hoping you help me figure out what I'm doing wrong with the mocking here? I attempted to duplicate essentially what the streaming test was doing but the stream object doesn't receive seem events when sending mock data to the http module.

(Also: see a number of TODOs in my code to see where I'm going with this, and for some open questions you might be able to shed some light on. There is certainly a bit of code duplication here I will work on refactoring out once I can get things working properly.)

@parroty
Copy link
Owner

parroty commented Jan 25, 2015

Thanks, the benchmark is great. I don't have clear idea about the mocking yet, but will be looking into it later (as I don't have my laptop with me now).
Sorry about the poor performance part. At least the JSON and struct convert part would not be good which does conversion twice. Maybe leaving as raw map or using poison library to directly converting to struct helps some.

@parroty
Copy link
Owner

parroty commented Jan 27, 2015

The issue seems reproduced when the pids of ExTwitter.stream_filter (defp mock_stream) at and Enum.take (bench) are different. The stream returned by ExTwitter.stream_filter is internally using local message box of pid where ExTwitter.stream_filter is called, and Enum.take from different pid seems failing.

It means the information is not isolated in the stream (not good) and I'll try to fix (not sure yet, but maybe through separate gen_server for storing tweets), but tentative solution might be calling both from the bench block.

diff --git a/bench/stream_bench.exs b/bench/stream_bench.exs
index 5fb0a4b..458ec67 100644
--- a/bench/stream_bench.exs
+++ b/bench/stream_bench.exs
@@ -36,7 +36,10 @@ defmodule BasicBench do

   # actual benchmark iteration, process tweet message from the streaming API.
   # mocks the stream ahead of time so mock setup doesn't affect benchmark time.
-  bench "stream tweets", [stream: mock_stream()] do
+  bench "stream tweets", [mock: mock_stream()] do
+    # create stream
+    stream = ExTwitter.stream_filter(track: "twitter", receive_messages: true)
+    :timer.sleep(100) # put small wait for async, as per _stream_test.exs
     send_mock_data(TestHelper.TestStore.get, @mock_tweet_json)
     tweet = Enum.take(stream, 1) |> List.first
     # TODO[bench]: remove below line for actual benchmarking once we get working
@@ -52,11 +55,6 @@ defmodule BasicBench do

     # mock oauth so when we create the stream it uses the mocked post method
     mock_oauth!
-
-    # create stream
-    stream = ExTwitter.stream_filter(track: "twitter", receive_messages: true)
-    :timer.sleep(100) # put small wait for async, as per _stream_test.exs
-    stream
   end

   defp mock_oauth! do

@mroth
Copy link
Contributor Author

mroth commented Jan 27, 2015

Oh interesting. Hmm. The problem with putting the stream creation in the bench block is it will be considered part of the benchmark, when in order to benchmark we need to isolate the time just to parse a single message.

This may be more a limitation of the benchfella library than an issue with extwitter. I'll see if I can figure out another way to get the data in..

@mroth
Copy link
Contributor Author

mroth commented Jan 27, 2015

Okay, I think I have an idea on how to do this via mocking input to the process_stream function directly, will report back soon.

@mroth mroth force-pushed the bench branch 3 times, most recently from 2036fbc to 7188bdd Compare January 27, 2015 22:52
@mroth
Copy link
Contributor Author

mroth commented Jan 27, 2015

Okay, made a little progress after banging my head on this for 5 hours. I rebased on the 0.4.0 changes in master as well.

Now the way it works is much simpler, because I made two methods public in the ExTwitter.API.Streaming module and hid them with @doc false, so that we could test them externally. If you look the benchmark code is much simpler now.

It somewhat works:

$ mix bench       
bench/stream_bench.exs:37: warning: function mock_out_tweet_parsing!/0 is unused
Settings:
  duration:      1.0 s
  mem stats:     false
  sys mem stats: false

[17:52:31] 1/3: ExTwitterStreamBench.parse tweet message - tweet
[17:52:33] 2/3: ExTwitterStreamBench.parse tweet message - control
[17:52:35] 3/3: ExTwitterStreamBench.process stream - single chunk
Finished in 6.79 seconds

ExTwitterStreamBench.parse tweet message - control:      50000   28.57 µs/op
ExTwitterStreamBench.parse tweet message - tweet:         5000   350.59 µs/op
ExTwitterStreamBench.process stream - single chunk:       5000   470.92 µs/op

What I still need to do here is:

  • figure out how to properly mock out the tweet parsing from the process stream test, so that the recursive loop there can be tested in isolation (I'm not sure how much overhead spawning the process inline adds, but hopefully not enough that I won't be able to see differences with the switching algorithm in there)
  • create a multi-part benchmark for the process stream, so I can see how it works in large messages that might come in in many chunks.

@parroty
Copy link
Owner

parroty commented Jan 28, 2015

Thanks, it's great. Shall I merge with this commit? (Some of the remaining items might be continuous efforts). Once it becomes good set of commits, please indicate so.

@mroth
Copy link
Contributor Author

mroth commented Jan 28, 2015

I still need to figure out how to mock out the tweet JSON parsing. I'm having trouble getting meck to work the way I would expect it to...

@@ -102,7 +103,7 @@ defmodule ExTwitter.API.Streaming do
is_end_of_message(part) ->
message = Enum.reverse([part|acc])
|> Enum.join("")
|> parse_tweet_message(configs)
|> __MODULE__.parse_tweet_message(configs)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this change, which was required to get Meck working for this.

@mroth
Copy link
Contributor Author

mroth commented Jan 28, 2015

Okay, I think this is pretty much working now. You might want to give my code a review to make sure I didn't do anything dumb.

Note that in order to properly stub out the tweet parsing for benchmark isolation, the benchmark scripts must now be run individually instead of in aggregate via mix bench, since benchfella doesn't currently support a teardown phase. (I did request it however in alco/benchfella#11, so if it gets added in the future we can take advantage of it.) I put a warning when that script is run just to make sure people note that.

Sample output (note these number should look different from earlier output since I'm working on a much faster desktop machine right now):

$ mix bench bench/stream_parse_bench.exs 
Settings:
  duration:      1.0 s
  mem stats:     false
  sys mem stats: false

[13:37:55] 1/2: ExTwitterStreamParseBench.parse tweet message - control
[13:37:59] 2/2: ExTwitterStreamParseBench.parse tweet message - tweet
Finished in 5.04 seconds

ExTwitterStreamParseBench.parse tweet message - control:     500000   6.22 µs/op
ExTwitterStreamParseBench.parse tweet message - tweet:        10000   110.56 µs/op

$ mix bench bench/stream_process_bench.exs 
Settings:
  duration:      1.0 s
  mem stats:     false
  sys mem stats: false

[13:38:07] 1/2: ExTwitterStreamProcessBench.process stream - single chunk
WARNING! We just stubbed ExTwitter.API.Streaming.parse_tweet_message
This stub will be effect until this process ends.

If you are running multiple benchmarks, these ones should be run
independently of others, you can specify the files to run directly via
`mix bench bench/filename_bench.exs` etc.

[13:38:09] 2/2: ExTwitterStreamProcessBench.process stream - multi chunk
Finished in 4.5 seconds

ExTwitterStreamProcessBench.process stream - single chunk:     100000   18.36 µs/op
ExTwitterStreamProcessBench.process stream - multi chunk:       50000   39.81 µs/op

There is still some extra overhead in the stream process benchmarks due to spawning the process but I believe it's minimal enough now that these benchmarks can be used to aide any performance oriented refactoring (and provide a framework for adding more benchmarks as needed).

Please let me know any comments/suggestions/concerns, and if you'd like me to squash the commits I can do that as well.

Thanks for your help and for the great library!

@mroth mroth changed the title Start of benchmark suite (in progress, feedback needed) Start of benchmark suite Jan 28, 2015
@parroty
Copy link
Owner

parroty commented Jan 29, 2015

Thanks, it's great. I can merge, but maybe it's good to squash as you mentioned. They comprise a good meaningful set of commits for this PR item.

see notes in bench/README.md regarding some caveats introduced with the
current benchmark setup

requires minor restructuring of some code to enable stubbing
@mroth
Copy link
Contributor Author

mroth commented Jan 29, 2015

Okay, I squashed this down to two commits now (I preserved the modification of the mix file to introduce the benchfella dependency as separate from the introduction of the benchmarks).

parroty added a commit that referenced this pull request Jan 29, 2015
Start of benchmark suite
@parroty parroty merged commit 8c0b7ef into parroty:master Jan 29, 2015
@mroth mroth deleted the bench branch January 29, 2015 02:50
@parroty
Copy link
Owner

parroty commented Jan 29, 2015

Thanks! I'll be experimenting some with the bench.
Also, I appreciate if you find any idea on performance improvements.

@mroth
Copy link
Contributor Author

mroth commented Jan 29, 2015

I'll dig in. I am not sure if the current benchmarks even represent the biggest areas for improvement, they were just my guesses based on my experiences with ttezel/twit#150 and what I saw in other clients in the http://github.com/mroth/twitter-streaming-showdown tests.

A good place to start might also be using eprof or something like that to find the hot spots during streaming then write some benchmarks around that to experiment. I don't have any real experience with Erlang code profiling but I'll try to give it a shot if I have some spare time next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants