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

Split regex_t to one per thread #273

Merged
merged 1 commit into from
Jun 6, 2017

Conversation

marks-sortable
Copy link
Contributor

When using carbon-c-relay to perform metric aggregation, strace found a lot of calls to futex. Using gdb, I discovered that we were frequently blocking on a mutex in the regex matching operations. By compiling a separate regex_t for each dispatcher, we can avoid blocking when matching metric names.

  * Export `workercnt` so we can create the correct number of `regex_t`s
  * Thread `dispatcher_id` down to where it's needed
  * Gives a 20-30% speed improvement in benchmarks
@grobian
Copy link
Owner

grobian commented May 10, 2017

Thanks for the analysis and patch!

Quite stupid if you ask me, since the manpage reads The compiled form is not altered during execution of regexec(), so a single compiled RE can be used simultaneously by multiple threads. But that is on Darwin, Linux manpages don't mention any such thing, except that posix declares it thread-safe/mt-safe. I guess they implemented that with a mutex, yay.

I'll have a look, I might modify your work here and there, for I don't like the globals, so I don't think we should introduce new ones.

@marks-sortable
Copy link
Contributor Author

You're welcome!

Here's somebody else also getting caught off-guard by the internal locks back in 2010: https://sourceware.org/bugzilla/show_bug.cgi?id=11159

@tehlers320
Copy link

tehlers320 commented Jun 5, 2017

FYI, i pulled this commit and 274 into v3.1 branch locally and created 3.1.1 pushing it out for testing. So far no problems and no locks. But like i said before our lock up issue seems mitigated by adding excessive threads to our instances (8).

However i do see a big bump in CPU use which i thought i'd share. CPU use increases are fine for me.
%user is shown
screen shot 2017-06-05 at 9 04 19 am

edit: actually this is not reliable data thinking it over more. We had a mirror setting go in with this change. This probably needs a lab environment for testing unless the below is not expected to jump the CPU.

"new-cluster" is an any_of match.

aggregate ^something
  some aggregate
  send to 
     old-cluster
     new-cluster
     stop
  ;

... 7 more aggregates

@grobian
Copy link
Owner

grobian commented Jun 5, 2017

I'm working on this PR at the moment. I'm unhappy about some assumptions done in the code, but I guess at some point I need to chew on those and just merge it.

@grobian grobian merged commit 2c621d4 into grobian:master Jun 6, 2017
grobian added a commit that referenced this pull request Jun 6, 2017
grobian added a commit that referenced this pull request Jun 7, 2017
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.

3 participants