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 options to allow overriding SendBufferSize pre process name prefix #772

Merged
merged 1 commit into from
Mar 20, 2020

Conversation

lingyuan2014
Copy link
Contributor

@lingyuan2014 lingyuan2014 commented Mar 19, 2020

This adds the ability to override SendBufferSize per process name prefix. I'm using prefix instead of regex because prefix seem to be sufficient for most usecases and is significantly faster:

➜  regexp_test go test -v -bench=.
goos: darwin
goarch: amd64
pkg: github.com/fiibbb/regexp_test
BenchmarkRegex
BenchmarkRegex-12                5526828               208 ns/op               0 B/op          0 allocs/op
BenchmarkPrefix
BenchmarkPrefix-12              344802882                3.46 ns/op            0 B/op          0 allocs/op
BenchmarkSuffix
BenchmarkSuffix-12              369626205                3.21 ns/op            0 B/op          0 allocs/op
BenchmarkContains
BenchmarkContains-12            142756165                8.41 ns/op            0 B/op          0 allocs/op
PASS
ok      github.com/fiibbb/regexp_test   7.896s

@CLAassistant
Copy link

CLAassistant commented Mar 19, 2020

CLA assistant check
All committers have signed the CLA.

@lingyuan2014 lingyuan2014 force-pushed the lingy/sendbufchan-override branch from be7c17e to 08279ce Compare March 19, 2020 23:59
@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #772 into dev will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #772      +/-   ##
==========================================
+ Coverage   88.64%   88.87%   +0.23%     
==========================================
  Files          41       41              
  Lines        4140     4145       +5     
==========================================
+ Hits         3670     3684      +14     
+ Misses        356      349       -7     
+ Partials      114      112       -2     
Impacted Files Coverage Δ
connection.go 88.73% <100.00%> (-0.35%) ⬇️
mex.go 80.56% <0.00%> (+0.94%) ⬆️
outbound.go 91.22% <0.00%> (+2.92%) ⬆️
root_peer_list.go 100.00% <0.00%> (+8.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90ac437...4f86f67. Read the comment docs.

@lingyuan2014 lingyuan2014 requested a review from alxn March 20, 2020 00:03
Copy link
Contributor

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Some minor comments, but looks good overall

connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
connection_test.go Outdated Show resolved Hide resolved
connection_test.go Show resolved Hide resolved
@lingyuan2014 lingyuan2014 force-pushed the lingy/sendbufchan-override branch 6 times, most recently from 748c546 to cd0585b Compare March 20, 2020 04:39
@lingyuan2014 lingyuan2014 requested a review from prashantv March 20, 2020 04:43
Copy link
Contributor

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Looks good, when looking over the test cases, using a slice is definitely the right move, since the logic is deterministic. With a map[string]int, if you had both abc and abcd, we'd have non-deterministic results since map iteration order is undefined.

Left some small cleanup comments on the test, but otherwise LGTM.

connection_test.go Outdated Show resolved Hide resolved
connection_test.go Show resolved Hide resolved
connection_test.go Outdated Show resolved Hide resolved
connection_test.go Outdated Show resolved Hide resolved
connection_test.go Outdated Show resolved Hide resolved
Copy link

@jaricftw jaricftw left a comment

Choose a reason for hiding this comment

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

LGTM, just nits to improve

connection.go Outdated Show resolved Hide resolved
@lingyuan2014 lingyuan2014 force-pushed the lingy/sendbufchan-override branch from cd0585b to 62889a9 Compare March 20, 2020 17:30
connection_test.go Outdated Show resolved Hide resolved
@lingyuan2014 lingyuan2014 force-pushed the lingy/sendbufchan-override branch from 62889a9 to cac7a5f Compare March 20, 2020 17:57
@lingyuan2014 lingyuan2014 force-pushed the lingy/sendbufchan-override branch from cac7a5f to 4f86f67 Compare March 20, 2020 18:07
@lingyuan2014 lingyuan2014 merged commit 41382bf into dev Mar 20, 2020
@prashantv prashantv deleted the lingy/sendbufchan-override branch March 20, 2020 21:28
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.

6 participants