<regex>: Implement small buffer optimization to store loop state#6092
<regex>: Implement small buffer optimization to store loop state#6092muellerj2 wants to merge 1 commit intomicrosoft:mainfrom
<regex>: Implement small buffer optimization to store loop state#6092Conversation
| _Allocation_guard& operator=(const _Allocation_guard&) = delete; | ||
| _Allocation_guard& operator=(_Allocation_guard&&) = delete; | ||
|
|
||
| ~_Allocation_guard() noexcept { |
There was a problem hiding this comment.
Isn't noexcept by default here in this case?
There was a problem hiding this comment.
I think I just copied this from some guard in <vector>.
There was a problem hiding this comment.
Then that's right probably.
| _Tgt_state_t<_It> _Tgt_state; | ||
| _Tgt_state_t<_It> _Res; | ||
| vector<_Loop_vals_v3_t<_Iter_diff_t<_It>>> _Loop_vals; | ||
| _Rx_fixed_size_buffer<_Loop_vals_v3_t<_Iter_diff_t<_It>>> _Loop_vals; |
There was a problem hiding this comment.
I assume we're not between releases and matcher doesn't need to be renamed again
There was a problem hiding this comment.
That`s what I took from STL's remarks on Discord that he would like to audit no_unique_address for the 14.51 release: The branch-off is imminent, but hasn't happened yet.
There was a problem hiding this comment.
That's correct. As long as we land this soon-ish, we're still covered by the _Matcher3 rename.
| _Guard._Ptr = nullptr; | ||
| } | ||
|
|
||
| _Compressed_pair<_Alloc, _Val> _Mypair; |
There was a problem hiding this comment.
It may be the time to use no unique address
There was a problem hiding this comment.
no_unique_address is C++20. I think [[msvc::no_unique_address]] is supported in older C++ dialects as well as an extension, but then we need a decision that we will make use of this extension.
There was a problem hiding this comment.
I don't know if it's supported downlevel by MSVC, Clang, and EDG. (_MSVC_NO_UNIQUE_ADDRESS is currently not set up to activate downlevel.) There's also a concern with CUDA.
I think messing with it here is too much risk for too little value. I'd rather stick with _Compressed_pair.
| return _Size * sizeof(_Ty); | ||
| } else { | ||
| _Allocate_buf(_Size); | ||
| return 0U; |
There was a problem hiding this comment.
The suffix is misleading. Just 0.
(There's a pointer-size-dependent suffix I think, but we can't use that here due to C++14 at least)
|
|
||
| _Matcher3<_BidIt, _Elem, _RxTraits, _It, void> _Mx( | ||
| _First, _Last, _Re._Get_traits(), _Re._Get(), _Re.mark_count() + 1, _Re.flags(), _Flgs); | ||
| alignas(_Loop_vals_v3_t<_Iter_diff_t<_It>>) unsigned char _Stackbuf[4096]; |
There was a problem hiding this comment.
What is this alignment?
There was a problem hiding this comment.
Usually it's the same as a pointer, except if some user passes an iterator where _Iter_diff_t<_It> requires some stricter alignment (although I don't see why anyone would want to do that).
StephanTLavavej
added
performance
As far as I'm concerned, |
3 participants
Disclaimer: Low-level memory management is not my forte, because I mostly write Java code in my day-to-day job.
Towards #5969.
This implements a minimal fixed-size buffer to support some small buffer optimizations during regex matching. In the first step, this fixed-size buffer is only used for the loop state storage (because this requires minimal changes in the matcher), but I plan to use it to store the state of capturing groups as well in follow-up PRs. For frames, we rather need a vector-like data structure that can grow but uses stack storage when the number of frames is few.
The fixed-size buffer takes an allocator in preparation for #174.
Since I want to use some common storage space on the stack for all of these buffers, the stack storage is not an internal member of the buffers but supplied as a pointer to them from the outside. The fixed-size buffer reports back to what extent it has made use of the stack storage. (We don't need this in this PR yet, because the stack storage is only by a single fixed-size buffer. But this will change when more buffers make use of the stack storage.)
The stack storage is also not a member of
_Matcher3so that we can adjust its size in the future without having to rename the class.I wasn't sure what exception to throw if too much memory is requested from the fixed-size buffer, but then went with
regex_error(error_stack)because I would like to avoid exceptions not of typeregex_errorand the description oferror_stackis "There was insufficient memory to determine whether the regular expression could match the specified character sequence." (The other candidate iserror_space. But it is described as "There was insufficient memory to convert the expression into a finite state machine.", so it appears to be intended for parsing and not matching.)I do wonder whether we should call
std::launder()when repurposing the stack storage (here, specifically, in_Rx_fixed_size_buffer::_Use_external_buf()). But it seems this isn't done elsewhere in the STL, including_Optimistic_temporary_buffer.Benchmarks
I highlighted the relevant lines in
regex_searchwhere the matcher uses some loop state. (Theregex_matchbenchmark is inconclusive, the difference vanishes in the noise on my machine.)