Skip to content

expression: implement vectorized evaluation for builtinInetNtoaSig#12088

Merged
sre-bot merged 15 commits intopingcap:masterfrom
Reminiscent:builtinInetNtoaSig
Sep 10, 2019
Merged

expression: implement vectorized evaluation for builtinInetNtoaSig#12088
sre-bot merged 15 commits intopingcap:masterfrom
Reminiscent:builtinInetNtoaSig

Conversation

@Reminiscent
Copy link
Contributor

@Reminiscent Reminiscent commented Sep 9, 2019

What problem does this PR solve?

Implement vectorized evaluation for builtinInetNtoaSig.

What is changed and how it works?

BenchmarkVectorizedBuiltinFunc/builtinInetNtoaSig-VecBuiltinFunc-4                300000              5391 ns/op
BenchmarkVectorizedBuiltinFunc/builtinInetNtoaSig-NonVecBuiltinFunc-4             100000             18383 ns/op

Check List

Tests

  • Unit test

@codecov
Copy link

codecov bot commented Sep 9, 2019

Codecov Report

Merging #12088 into master will decrease coverage by 0.0521%.
The diff coverage is 52%.

@@               Coverage Diff                @@
##             master     #12088        +/-   ##
================================================
- Coverage   81.3875%   81.3354%   -0.0522%     
================================================
  Files           452        451         -1     
  Lines         96618      96584        -34     
================================================
- Hits          78635      78557        -78     
- Misses        12363      12397        +34     
- Partials       5620       5630        +10

result.AppendNull()
continue
}
ip := make(net.IP, net.IPv4len)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we reuse this buffer during for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update. PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Any benchmark result update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update. But it seems that the promotion is not obvious.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use option benchmem to see the benchmark of memory.

go test -benchmem

Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 9, 2019
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 10, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 10, 2019

/run-all-tests

@sre-bot sre-bot merged commit e8f63be into pingcap:master Sep 10, 2019
@Reminiscent Reminiscent deleted the builtinInetNtoaSig branch December 24, 2019 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants