Skip to content

http: minor perf fix, not copying strings#6861

Merged
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
alyssawilk:string
May 8, 2019
Merged

http: minor perf fix, not copying strings#6861
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
alyssawilk:string

Conversation

@alyssawilk
Copy link
Contributor

We're good for this, right?

Risk Level: Low
Testing: existing tests pass
Docs Changes: n/a
Release Notes: n/a

alyssawilk added 2 commits May 8, 2019 12:40
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@mattklein123
Copy link
Member

If you have this fixed internally (!!!), I think there are a lot more examples than these but I don't think we have been tracking them carefully.

@jmarantz
Copy link
Contributor

jmarantz commented May 8, 2019

I also have this fixed differently in #6733 though I might not have pushed that change yet.

Basically the strings in this need to become StatNames so they can be used to quickly joined into larger StatNames. Checking this in now is fine if urgent; I can deal with the merge conflict later.

@alyssawilk
Copy link
Contributor Author

It's not urgent but yeah, in your current version I still see the std::strings and TODOs and I don't see the test change?

@alyssawilk
Copy link
Contributor Author

Yeah, FWIW I did a quick grep for const std::string not in a function signature and it didn't help me find other things to clean up. I wonder if I can turn up more poking around our import logs and PR history for "string"

@mattklein123
Copy link
Member

Yeah, FWIW I did a quick grep for const std::string not in a function signature and it didn't help me find other things to clean up. I wonder if I can turn up more poking around our import logs and PR history for "string"

If you can somehow search GH comments for me saying "sigh" you will probably fine some. :)

@jmarantz
Copy link
Contributor

jmarantz commented May 8, 2019

Yeah it's not fully compiling yet but I pushed it anyway just now to make it visible.

https://github.com/envoyproxy/envoy/pull/6733/files#diff-a00f27eaffcd28ed6b74102912eb1d65L219

not ready for review obviously but early feedback on the approach would be great.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Yes, please remove all of these Google string hacks. We might have a few around string_view, but I think these are not in the protobuf space.

@mattklein123 mattklein123 merged commit f0f22f0 into envoyproxy:master May 8, 2019
@alyssawilk alyssawilk deleted the string branch December 9, 2019 21:10
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.

5 participants