Arena; avoid use of temporary.#10940
Conversation
|
Still parsing the code, I thought this wasn't doing a heap allocation so I closed the PR, but in fact it does.
|
| if (info->active) { | ||
| if (info->active->last_failure.load() == TS_TIME_ZERO) { | ||
| char *url_str = t_state.hdr_info.client_request.url_string_get(&t_state.arena, nullptr); | ||
| char *url_str = t_state.hdr_info.client_request.url_string_get_ref(nullptr); |
There was a problem hiding this comment.
Will this reserve space for a copy of the URL every time it is called? Can we be sure it will only be called once?
Should there be a swoc::AbstractMemArena, that MemArena inherits from, and url_string_get() takes a reference to? Then there could be a swoc::LocalMemArena that inherits from swoc::AbstractMemArena, that would have an internal array, small enough so that LocalMemArena could reasonably be a stack variable? That seems like it might be the ticket for this use case.
There was a problem hiding this comment.
The copy on the heap is a cache and it's not duplicated if already present. If it's already there, url_string_get_ref will return that existing copy. Therefore in the worst case it's the same, and the best case it's very fast because the URL doesn't have to be printed. The original code used Arena as temporary space, but as noted that doesn't avoid the copy to the heap.
This isn't really done as I'd like, but changing things to do the better style is a very heavy lift (e.g., this should be returned TextView not char const *).
MemArena already supports the use of static blocks for memory allocation. Speaking of which, that would be good in another PR I'm working on for which this is a precursor.
ywkaras
left a comment
ywkaras
left a comment
There was a problem hiding this comment.
No net increase in the scariness of this logic.
2 participants
I can't see any need for the temporary, when the reference can be used (since it is used only for the debug message). Further,
url_string_getprints the URL into the heap also, so either it's already there and it's faster to use it, or it will be generated.