Issue #286: use string_view in API methods where applicable #288

stuart-byma · 2022-10-29T14:32:37Z

Issue or RFC Endorsed by Maintainers

Description of the Change

This change reduces required copying of immutable data primarily in the HTTP request API. Methods returning strings now return string_view to avoid copying immutable data from the underlying MHD impl. get_*s methods still return a std::map which must be allocated and constructed, but does not copy underlying strings.

Fix a few instances of these method calls in the code base.

Introduce some type aliases for maps from string_view to string_view, string to string to reduce code verbosity.
Overload the comparator operator()s as required.

Provide overloads for dump_*_map functions and a template impl to avoid code duplication.

Bump C++ version to 17 required to use string_view. Remove CI configurations using ancient compilers that don't support string_view or C++17

Alternate Designs

No other alternatives. The change is relatively simple.

Possible Drawbacks

This is a potentially breaking change to the API. Users may have to update their apps after updating.

Verification Process

All tests passing.

Release Notes

Many instances of std::string within the API changed to std::string_view to improve performance by reducing copies and memory allocations.

This change reduces required copying of immutable data primarily in the HTTP request API. Methods returning strings now return string_view to avoid copying immutable data from the underlying MHD impl. get_*s methods still return a std::map which must be allocated and constructed, but does not copy underlying strings. Fix a few instances of these method calls in the code base. Introduce some type aliases for maps from string_view to string_view, string to string to reduce code verbosity. Overload the comparator operator()s as required. Provide overloads for dump_*_map functions and a template impl to avoid code duplication. Bump C++ version to 17 required to use string_view. Remove CI configurations using ancient compilers that don't support string_view or C++17

etr

Mostly minor comments from me - sorry for the delay.

src/httpserver/http_utils.hpp

etr · 2022-11-08T00:05:10Z

src/httpserver/http_utils.hpp

 **/
-void dump_header_map(std::ostream &os, const std::string &prefix, const std::map<std::string, std::string, header_comparator> &map);
+void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map &map);
+void dump_header_map(std::ostream &os, const std::string &prefix, const http::value_map_view &map);


I would go one way or the other for this dump_* methods and not keep both versions. These are mostly for debug purposes.

I would agree, but the problem is that the HttpResponse type owns its header/footer data and therefore has value_maps and not value_map_views. So there are places where dump_header_map is used on both owning and non-owning map types.

I wouldn't bother trying to produce a "view" version of this one as it is a method used for debugging only. Internally to the library only http_response and http_request use it (both in their internal operations) and a test for http_utils that builds the map as map of strings. I think just keeping the old ones here is fine - outside of this PR, I'll move this under "details" to make it clear that it is not something we offer as interface, anyway.

I actually left just the view version. The test case usages are switched to view maps (trivial) which left HttpResponse as the only place data owning versions were being dumped. I added utility to convert the map to a view just for that case. I think this results in the fewest lines of code changed. The utility (to_view_map) can also just go in the details namespace when you make that change.

src/webserver.cpp

src/httpserver/http_utils.hpp

This change reduces required copying of immutable data primarily in the HTTP request API. Methods returning strings now return string_view to avoid copying immutable data from the underlying MHD impl. get_*s methods still return a std::map which must be allocated and constructed, but does not copy underlying strings. Fix a few instances of these method calls in the code base. Introduce some type aliases for maps from string_view to string_view, string to string to reduce code verbosity. Overload the comparator operator()s as required. Provide overloads for dump_*_map functions and a template impl to avoid code duplication. Bump C++ version to 17 required to use string_view. Remove CI configurations using ancient compilers that don't support string_view or C++17

etr · 2022-11-08T23:26:00Z

src/http_request.cpp


-const std::map<std::string, std::string, http::arg_comparator> http_request::get_args() const {
-    std::map<std::string, std::string, http::arg_comparator> arguments;
+const http::arg_map http_request::get_args() const {


would it make sense for this to return a map of view as for the rest?

Actually ran into an issue with this. build_request_args actually makes a temp copy of the underlying data and does some unescape stuff on it. We cannot return a view over that temp data since it goes out of scope.

uhm...that is a really annoying inconsistency in the interface that will leave folks puzzled. I need to think of whether there is a way out (philosophically, I'd rather prioritize clarity/consistency over performance, unless major)

To be fair, all of this originates from some old "unescaping" logic that made us bypass the mhd unescaper and build a separated unescaping logic on top of it. I think we might just have to unblock that separately (and I think it should be possible, given that nobody has touched that code in the past 10 years and existed to fix some misbehavior that was there in a dependency at that time).

Agree that can be handled as a separate issue. Afterward we can make get_args return an arg_view_map.

Do we think the MHD unescaper is fine at this point? Maybe worth checking.

get_args firsts takes a copy of the args member var, then goes to MHD to get additional args. Can we use the args member var to just cache the unescaped values from MHD, and then return an arg_view_map constructed from the args member var?

Sorry for the barrage of comments, but I notice get_arg to get a single arg does not do any unescaping, why is that?

Sorry for the barrage of comments, but I notice get_arg to get a single arg does not do any unescaping, why is that?

It sounds very much like a bug. To be fair, I can see there is some old code in here that makes no sense anymore (see it tries to get data from an "args" dataset that never gets filled because the method is const). I think the general management of args and unescaping has issues that need fixing.

etr · 2022-11-08T23:27:41Z

README.md

+* _**const std::map<std::string_view, std::string_view, http::header_comparator>** get_headers() **const**:_ Returns a map containing all the headers present in the HTTP request.
+* _**const std::map<std::string_view, std::string_view, http::header_comparator>** get_cookies() **const**:_ Returns a map containing all the cookies present in the HTTP request.
+* _**const std::map<std::string_view, std::string_view, http::header_comparator>** get_footers() **const**:_ Returns a map containing all the footers present in the HTTP request (only for http 1.1 chunked encodings).
+* _**const std::map<std::string_view, std::string_view, http::arg_comparator>** get_args() **const**:_ Returns all the arguments present in the HTTP request. Arguments can be (1) querystring parameters, (2) path argument (in case of parametric endpoint, (3) parameters parsed from the HTTP request body if the body is in `application/x-www-form-urlencoded` or `multipart/form-data` formats and the postprocessor is enabled in the webserver (enabled by default).


This doesn't match the implementation below (which still seems not to be using views)

Updated. I think I caught all instances, but if you can double check that would be great.

This change reduces required copying of immutable data primarily in the HTTP request API. Methods returning strings now return string_view to avoid copying immutable data from the underlying MHD impl. get_*s methods still return a std::map which must be allocated and constructed, but does not copy underlying strings. Fix a few instances of these method calls in the code base. Introduce some type aliases for maps from string_view to string_view, string to string to reduce code verbosity. Overload the comparator operator()s as required. Bump C++ version to 17 required to use string_view. Remove CI configurations using ancient compilers that don't support string_view or C++17

In the file upload test, it was saving the arg_view_map from the request, which went out of scope, leaving dangling refs. Fix by making a deep copy.

Because build_request_args makes a temp copy of underlying data and mutates it, it is not feasible currently to return a view over that data.

etr · 2022-11-11T18:25:22Z

src/httpserver/http_request.hpp

      * @return the value of the header.
     **/
-     const std::string get_header(const std::string& key) const;
+     std::string_view get_header(std::string_view key) const;


Should we align the rest of the public interface of this class to use string_view? (e.g. get_user, get_pass, get_method, etc.)

I think that would be best. Design decisions to make though: some of these other methods modify data extracted from MHD (e.g. get_querystring), construct a result and then return it. I see two options here. One is to change the signature and/or return values to match whatever MHD gives, and return views over that data. The other is to construct the result as now only once, cache in the request object, and return a view over that (also if the user calls twice, the cached value is simply returned on the second call). Thoughts?

It's unfortunate that I am not sure I really like any of the five options I see for this. We basically can:

Leave the logic as is, and have an inconsistent interface. Some methods will return string, others return string_view. The issue with it is that basically we are basically projecting outside our own internal logic and the user pays the cost for this by having to adapt to what each method returns for a reason that is not immediately obvious when in their position.

Revert the methods to pure proxies over what libmicrohttpd returns. This comes at a loss of the extra simplicity that httpserver provides (which is one of the core reasons people use this library).

Pre-fetch all data from libmicrohttpd and store it preventively in the request object. This would allow to use string_view everywhere. This used to be the old implementation, but we moved away from it because it was just way too expensive for no reason (the user pays the cost of the fetch + copy even when they never use the data).

Do a fetch and cache. This is an implementation I had thought of when migrating off of the old prefetch approach. It has the extra advantage of making subsequent calls cheaper. It has a massive issue that it makes all getters non const which is a really annoying property for the interface of what basically is a class meant to store and transport data. I considered and discarded this approach when moving away from the old pre-fetch one.

Just leave the class as is and not use string_view at all.

I am currently pondering 1 an 5 - everything else just doesn't seem worth it. Instinctively, I'd say 5 would be my general inclination because philosophically I'd rather prioritize clarity/consistency over marginal performance, but I can welcome disagreements, if well motivated. I also think it would be worth to simulate some heavy data fetch (e.g. of headers) with both approaches to actually prove that using string_view whenever possible is worth the inconsistency it costs interface-wise.

I agree that 3 is untenable performance-wise. Most users are not going to use every field, prefetch has a high init cost. 1 is also undesirable; I agree it is better to be as consistent as possible.

I am not sure I agree with your assessment of point 4. An object internal cache doesn't mean we have to sacrifice const on all the accessors. It just means we can't cache the items directly in the request object. We can use an private cache type, and store it within a unique_ptr. Then we have a mutable cache from within a const accessor. There are only a few values that need to be cached in this way for us to make the whole API return string_view.

I think I prefer this option, it seems to be the best compromise between API consistency, const-ness, and performance. I have already tried this out, I will add it to the PR and look forward to your thoughts.

That's a good idea - I am looking at the implementation and it seems like it lands in a good place. I've just left a few minor comments. Take a look at those. I'll take another re-read later to give you a more complete feedback.

Add a data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites.

src/httpserver/http_request.hpp

Add a http_request_data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites. Remove the existing http_request::args member, use the cache instead. Make the http_request class move-only.

src/http_request.cpp

src/http_response.cpp

etr · 2022-11-16T22:19:33Z

src/http_response.cpp

+namespace {
+static inline http::header_view_map to_view_map(const http::header_map & hdr_map) {
+    http::header_view_map view_map;
+    for (const auto & item : hdr_map) {


nit: keep '&' attached on the left

not sure you did this?

I noticed others below - can you make sure you keep the reference '&' consistently together (left attached) to the type in all of your changes? (same for pointers '*')

I am aware the code is inconsistent (even in some of the rows you are modifying), but let's make sure we fix everything we are touching or creating anew. I need to go and have a pass at cleaning everything up later for the rest..

I think I got them all now. I would suggest configuring something clang-format, though the initial application of it will likely make quite a few changes that are formatting-only.

Yep, there is a change pending on that (with a few thousands fixes) that I am going through.

src/http_utils.cpp

src/httpserver/http_request.hpp

etr · 2022-11-16T22:24:28Z

Only minor comments on the change at this point - I think once we close those, we are done.

Add a http_request_data_cache type to the http request type, and a unique_ptr instance member. This allows caching of transformed and/or copied data from MHD, while preserving const-ness of all accessor methods. Change all applicable methods to string_view, fix a few call sites. Remove the existing http_request::args member, use the cache instead. Make the http_request class move-only.

etr · 2022-11-17T21:11:37Z

Thanks for going through with this. It is merged now!

etr reviewed Nov 8, 2022

View reviewed changes

stuart-byma added 5 commits November 9, 2022 09:04

remove accidentally added line of code

4d17763

fix data scope issue

dbef2a6

In the file upload test, it was saving the arg_view_map from the request, which went out of scope, leaving dangling refs. Fix by making a deep copy.

undo change from arg_map to arg_view_map

a391d14

Because build_request_args makes a temp copy of underlying data and mutates it, it is not feasible currently to return a view over that data.

fix readme after changing get_args signature

dbe1aab

etr reviewed Nov 11, 2022

View reviewed changes

stuart-byma added 4 commits November 14, 2022 09:29

stuart-byma closed this Nov 14, 2022

stuart-byma reopened this Nov 14, 2022