Skip to content

Conversation

@Arzaghi
Copy link
Contributor

@Arzaghi Arzaghi commented Aug 21, 2020

Fixes #1217

As @BillyONeal suggested here I think in the described situation, it is more reasonable to grow the vector to the maximum possible size according to the geometric growth speed.
Furthermore, if someone repeatedly increases vector size it is more likely that he/she wants to increase the size even more. So it is better to have the maximum preallocation space that prevents huge relocating data.
So I've changed it to max_size()

@Arzaghi Arzaghi requested a review from a team as a code owner August 21, 2020 19:26
@StephanTLavavej StephanTLavavej added bug Something isn't working performance Must go faster labels Aug 22, 2020
Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

Would you be interested in finding other affected places in the repo and implementing similar fixes there? (As @BillyONeal mentions in the issue, the same pattern is likely present in other places implementing geometric growth.) If you would rather not, we should probably file a subsequent issue to follow up on other occurrences after vector is fixed.

@Arzaghi
Copy link
Contributor Author

Arzaghi commented Aug 24, 2020

Would you be interested in finding other affected places in the repo and implementing similar fixes there? (As @BillyONeal mentions in the issue, the same pattern is likely present in other places implementing geometric growth.) If you would rather not, we should probably file a subsequent issue to follow up on other occurrences after vector is fixed.

Yes, I would. I will apply your suggestion on this change and fix other containers for the same issue.

@mnatsuhara Sorry I couldn't find anywhere else contain a similar issue. could you provide a clue about where else should I apply this fix?

@StephanTLavavej
Copy link
Member

basic_string already clamps to max_size:

STL/stl/inc/xstring

Lines 4257 to 4269 in 484fbc9

_NODISCARD static size_type _Calculate_growth(
const size_type _Requested, const size_type _Old, const size_type _Max) noexcept {
const size_type _Masked = _Requested | _ALLOC_MASK;
if (_Masked > _Max) { // the mask overflows, settle for max_size()
return _Max;
}
if (_Old > _Max - _Old / 2) { // similarly, geometric overflows
return _Max;
}
return (_STD max)(_Masked, _Old + _Old / 2);
}

After a quick search, I also don't see any other occurrences of geometric growth that need to be changed. (valarray grows exactly.) I searched for "geometric", "exponential", and \w+ \+ \w+ / 2. So I think this is sufficient, no need for a followup issue.

@StephanTLavavej StephanTLavavej self-assigned this Aug 26, 2020
@StephanTLavavej StephanTLavavej merged commit 1e26743 into microsoft:master Aug 26, 2020
@StephanTLavavej
Copy link
Member

Thanks for making our vector behavior consistent with string! 🎉

@Arzaghi Arzaghi deleted the Fix_Issue1217 branch August 27, 2020 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working performance Must go faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<vector>: vector doesn't comply with amortized time requirements near max_size

4 participants