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

Add regexp benchmark test #387

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add regexp benchmark test #387

wants to merge 2 commits into from

Conversation

colmsnowplow
Copy link
Collaborator

Making PR so I can leave comments - didn't think of the fact that I couldn't otherwise

for i := 0; i < numGoroutines; i++ {
go func() {
re.MatchString(line)
c <- 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mentioned when we discussed yesterday - channels are fairly expensive. What I didn't think to mention is you can use a waitgroup to do the same for cheaper - which would reduce the amount of possible skewing factors in the benchmark.

Not asking you to change it here - just following up with relevant info that I neglected to mention yesterday. I think what you have here measures things equally enough for our purposes.

}


// Results
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK interesting - these results indicate that there's negligible difference between the approaches. Which would mean that the implementation doesn't introduce an unnecessary bottleneck on filtering. Good to know!

I'll re-run on my machine to confirm too.

)

func Benchmark_OriginalPattern_Simple_Match(b *testing.B) {
regexToMatch, err := regexp.Compile(simpleRegexp)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is simpler than the switch function!

If you did want to generalise the repeated code here further, it could be achieved by passing b as a parameter. In this case I don't think it'd be necessary/worth the effort just to factor the code, but for future reference that's an option!

We use that pattern in snowman tests a lot - and it is worth the effort there, because we often want to do the same test-related things across many different tests. (Although that code base is quite complex - not something I'd point to as being a perfect example of simplification!)

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