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.

Sign up for GitHub

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

Jump to bottom

Use StringView for protocol stack to avoid calling strlen on strings with a known length. #1534

Merged
merged 1 commit into from
Mar 11, 2017
Merged

Use StringView for protocol stack to avoid calling strlen on strings with a known length. #1534

merged 1 commit into from
Mar 11, 2017

Conversation

SolidWallOfCode
Copy link
Member

This is an alternative proposal to #1520.

@SolidWallOfCode SolidWallOfCode self-assigned this Mar 4, 2017
@SolidWallOfCode SolidWallOfCode added this to the 7.2.0 milestone Mar 4, 2017
@SolidWallOfCode
Copy link
Member Author

I checked the compiler output and AFAICT passing a StringView by value is the same cost as passing two integers. That is, passing a StringView by value has the same performance as passing a pointer and a length by value.

@atsci
Copy link

atsci commented Mar 4, 2017

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/freebsd-github/1665/ for details.

@atsci
Copy link

atsci commented Mar 4, 2017

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/97/ for details.

SolidWallOfCode added a commit to SolidWallOfCode/trafficserver that referenced this pull request Mar 4, 2017
@SolidWallOfCode
PR apache#1534: A bit of cleanup.
fe2bf0d
@atsci
Copy link

atsci commented Mar 4, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1561/ for details.

SolidWallOfCode added a commit to SolidWallOfCode/trafficserver that referenced this pull request Mar 4, 2017
@SolidWallOfCode
PR apache#1534: A bit of cleanup.
3586fe4
SolidWallOfCode added a commit to SolidWallOfCode/trafficserver that referenced this pull request Mar 4, 2017
@SolidWallOfCode
PR apache#1534: Fix FreeBSD build problem.
abedcee
@atsci
Copy link

atsci commented Mar 4, 2017

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/freebsd-github/1666/ for details.

@atsci
Copy link

atsci commented Mar 4, 2017

clang-analyzer build failed! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/229/ for details.

@atsci
Copy link

atsci commented Mar 4, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1562/ for details.

@atsci
Copy link

atsci commented Mar 4, 2017

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/98/ for details.

@atsci
Copy link

atsci commented Mar 4, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1563/ for details.

@atsci
Copy link

atsci commented Mar 4, 2017

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/99/ for details.

@atsci
Copy link

atsci commented Mar 4, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1667/ for details.

@atsci
Copy link

atsci commented Mar 4, 2017

clang-analyzer build failed! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/230/ for details.

@atsci
Copy link

atsci commented Mar 4, 2017

clang-analyzer build failed! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/231/ for details.

SolidWallOfCode added a commit to SolidWallOfCode/trafficserver that referenced this pull request Mar 4, 2017
@SolidWallOfCode
PR apache#1534: Fix clang analyzer problems.
7d99ac3
@atsci
Copy link

atsci commented Mar 5, 2017

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/100/ for details.

@atsci
Copy link

atsci commented Mar 5, 2017

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1668/ for details.

@atsci
Copy link

atsci commented Mar 5, 2017

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1564/ for details.

@atsci
Copy link

atsci commented Mar 5, 2017

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/232/ for details.

@shukitchan
Copy link
Contributor

👍
Looks good.
Just one question. Should the internal "protocol_contains" functions return "StringView" as well?
Obviously we should not be changing the return type of public API. But internally should we be using "StringView" for those internal api calls?

@SolidWallOfCode
Copy link
Member Author

Hmmmm. I think I'd leave that for another day. Those aren't really used internally, only to forward the API request. The return value isn't examined except to check for nullptr which is just as fast without the StringView. Further the StringView can't be returned via the TS API without changes, making that not particularly useful. A key goal of that call is to normalize the string to allow direct pointer comparison, in which case the length is superflous.

@SolidWallOfCode
Copy link
Member Author

@bryancall says StringView should be changed to be conformant with string_ref from C++17. This would involve changing the method naming scheme and adding some additional methods to cover those in string_ref.

@SolidWallOfCode
Copy link
Member Author

Squashing and verifying.

@atsci
Copy link

atsci commented Mar 11, 2017

RAT check failed! https://ci.trafficserver.apache.org/job/RAT-github/53/

@atsci
Copy link

atsci commented Mar 11, 2017

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/40/

@atsci
Copy link

atsci commented Mar 11, 2017

AU check failed! https://ci.trafficserver.apache.org/job/autest-github/38/

@atsci
Copy link

atsci commented Mar 11, 2017

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/1735/

@atsci
Copy link

atsci commented Mar 11, 2017

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/164/

@atsci
Copy link

atsci commented Mar 11, 2017

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1630/

@SolidWallOfCode SolidWallOfCode merged commit a9a354a into apache:master Mar 11, 2017
@atsci
Copy link

atsci commented Mar 11, 2017

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/296/

@shukitchan shukitchan mentioned this pull request Mar 11, 2017
Closed
@SolidWallOfCode SolidWallOfCode mentioned this pull request Apr 6, 2017
Merged
SolidWallOfCode pushed a commit to SolidWallOfCode/trafficserver that referenced this pull request Apr 13, 2017
YTS-1189: Add protocol stack to VIA. (apache#425)
f2a447c 
Change protocol stack reporting to use StringView to avoid strlen calls.
See apache#1520, apache#1534
Cherry pick a9a354a
            ebd6815
@zwoop zwoop removed this from 7.x critical issues in 7.x releases Apr 18, 2017
@zwoop zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017
This was referenced May 29, 2017
Merged
Closed
@SolidWallOfCode SolidWallOfCode mentioned this pull request Jun 9, 2017
Closed
SolidWallOfCode added a commit to SolidWallOfCode/trafficserver that referenced this pull request Jun 9, 2017
@SolidWallOfCode
Via: Revert changes for apache#1534 and subsequent.
cbf65cb
@SolidWallOfCode SolidWallOfCode mentioned this pull request Jun 9, 2017
Merged
SolidWallOfCode added a commit to SolidWallOfCode/trafficserver that referenced this pull request Jun 9, 2017
@SolidWallOfCode
Via: Revert changes for apache#1534 and subsequent.
3ff60fd
SolidWallOfCode added a commit to SolidWallOfCode/trafficserver that referenced this pull request Jun 9, 2017
@SolidWallOfCode
Via: Revert changes for apache#1534 and subsequent.
e305b85
SolidWallOfCode added a commit to SolidWallOfCode/trafficserver that referenced this pull request Jun 9, 2017
@SolidWallOfCode
Via: Revert changes for apache#1534 and subsequent.
ecc945e
SolidWallOfCode added a commit to SolidWallOfCode/trafficserver that referenced this pull request Jun 11, 2017
@SolidWallOfCode
Via: Revert changes for apache#1534 and subsequent.
7f25bb0
SolidWallOfCode added a commit that referenced this pull request Jun 12, 2017
@SolidWallOfCode
Via: Revert changes for #1534 and subsequent.
7c9e456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maskit maskit maskit approved these changes

@zwoop zwoop zwoop approved these changes

@shukitchan shukitchan Awaiting requested review from shukitchan

Assignees

@SolidWallOfCode SolidWallOfCode

Labels
Core WIP
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@SolidWallOfCode @atsci @shukitchan @zwoop @mlibbey @maskit