Remove maps built on each call of some conversion functions.#3377
Conversation
|
|
||
| using namespace Aws::Utils; | ||
| HttpClientMetricsType GetHttpClientMetricTypeByName(const Aws::String& name) | ||
| static const Aws::Array<std::pair<HttpClientMetricsType, const char*>, 12> httpClientMetricsNames = |
There was a problem hiding this comment.
Not sure which casing I should use.
sbiscigl
left a comment
sbiscigl
left a comment
There was a problem hiding this comment.
One comment to use std::find_if to preserve old behavior, but outside of that, really appriciate you taking the time to address a TODO that we left. sorry that we left it, and thanks for taking the time to do it.
| } | ||
| return Aws::String(it->second.c_str()); | ||
| assert(httpClientMetricsNames[static_cast<int>(type)].first == type); | ||
| return Aws::String(httpClientMetricsNames[static_cast<int>(type)].second); |
There was a problem hiding this comment.
would prefer you to use the std::find lookup i.e.
const auto metricName = std::find_if(httpClientMetricsNames.begin(),
httpClientMetricsNames.end(),
[&type](const std::pair<HttpClientMetricsType, const char *>& nameEntry) -> bool { return nameEntry.first == type; });
if(metricName == httpClientMetricsNames.end()) {
return HTTP_CLIENT_METRICS_UNKNOWN;
}
return metricName->second;as this preserves the behavior from before. i recognize that the map logically doesnt have any illegal access, but this makes sure that there is a defined behavior for when none are found when assertions are disabled.
There was a problem hiding this comment.
Hi, I was unsure whether you wanted to prevent an array out of bound index, or to prevent someone adding some entries in the constant table not in the correct order.
I have added a check for this first case; I can of course put an std::find if you want to prevent against the second case.
There was a problem hiding this comment.
or to prevent someone adding some entries in the constant table not in the correct order.
yeah my main concern is that someone edits this out of order, creates a binary incompatibility and it shows up in downstream package managers. I think we can change it ourselves to std::find if we find that is the case down the road, and leave the PR as is for now
svart-riddare
force-pushed
the
remove-unexpected-memory-allocations
branch
from
87cc114 to
8eb2e6c
Compare
The said map should have been static as a 'TODO' in the code explains. Since the static keyword was removed when the `std::map` was changed to an `Aws::Map`, the map is rebuilt at each calls, which is a lot of work, and calls to `malloc`/`free`. Although the search is now linear instead of logarithmic, the number of entries is quite small and it's always better than rebuilding the map at each call. This saves about 300 mallocs per PutObject request for example.
sbiscigl
force-pushed
the
remove-unexpected-memory-allocations
branch
from
8eb2e6c to
332d9f0
Compare
3 participants
The said map should have been static as a 'TODO' in the code explains. Since the static keyword was removed when the
std::mapwas changed to anAws::Map, the map is rebuilt at each calls, which is a lot of work, and calls tomalloc/free. Although the search is now linear instead of logarithmic, the number of entries is quite small and it's always better than rebuilding the map at each call.This saves about 300 mallocs per PutObject request for example.
Issue #, if available:
Description of changes:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.