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

Use vector::reserve in bbox::parse to optimize allocations #280

Closed

Conversation

Woazboat
Copy link
Contributor

The strs vector in bbox::parse() usually requires 4 elements for parsing a bbox from a string. Reserve enough space in advance for the expected number of elements to avoid repeated (re-)allocations.

@Woazboat
Copy link
Contributor Author

Adding the reserve here might not actually matter. I did some benchmarks now and there isn't really any benefit. The current implementation is generally pretty inefficient. This version using C++ 20 range views is ~7.5 times faster for the normal case (~1200ns vs ~160ns) and ~200 times faster (~5000ns vs ~25ns) in the worst case when the string contains invalid doubles.

(Would obviously require C++20 though)

auto to_double(std::string_view s) -> std::optional<double>
{
    if (double value; std::from_chars(s.begin(), s.end(), value).ec == std::errc{})
        return value;
    else
        return std::nullopt;
};

bool bbox::parse(const std::string_view s) {
    std::array<double, 4> dbls;
    auto r = s |
        std::ranges::views::split(',') |
        std::ranges::views::transform([](auto&& r){
            return to_double(std::string_view{r.begin(), r.end()});
        });

    size_t i = 0;
    for (const auto& x : r) {
        if (i > 3 || !x)
            return false;

        dbls[i] = *x;
        ++i;
    }

    if (i != 4)
        return false;

    *this = bbox(dbls[1], dbls[0], dbls[3], dbls[2]);

    return true;
}
BM_bbox_parse_noreserve                                   1240 ns         1238 ns       638836
BM_bbox_parse_noreserve_invalid_toofewcoords               503 ns          502 ns      1327227
BM_bbox_parse_noreserve_invalid_toomanycoords             1006 ns         1005 ns       744590
BM_bbox_parse_noreserve_invalid_notadouble                5099 ns         5093 ns       100000
BM_bbox_parse_reserve                                     1224 ns         1222 ns       590153
BM_bbox_parse_reserve_invalid_toofewcoords                 490 ns          489 ns      1425225
BM_bbox_parse_reserve_invalid_toomanycoords               1039 ns         1038 ns       648569
BM_bbox_parse_reserve_invalid_notadouble                  5167 ns         5161 ns       100000
BM_bbox_parse_transform_array                              160 ns          160 ns      4696388
BM_bbox_parse_transform_array_invalid_toofewcoords         114 ns          114 ns      6191278
BM_bbox_parse_transform_array_invalid_toomanycoords        197 ns          197 ns      3410275
BM_bbox_parse_transform_array_invalid_notadouble          24.9 ns         24.9 ns     26614255

@tomhughes
Copy link
Contributor

Either way, parsing a bbox is done once per request at most and is a tiny part of the overall cost of any request so it's probably not a good target for spending a lot of time on optimisation.

@mmd-osm
Copy link
Collaborator

mmd-osm commented Nov 23, 2022

Thanks, we can't use C++20 right now, and as the propose code change has no performance impact, I'm inclined to close your pull request.

@mmd-osm mmd-osm closed this Nov 23, 2022
@Woazboat Woazboat mentioned this pull request Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants