Optimize big stack allocation (move to heap) #3016
Conversation
|
Stack allocation is free but heap isn't; how is this an optimization? |
Out of stack issues? https://docs.microsoft.com/en-us/visualstudio/code-quality/c6262?view=vs-2019 |
| // windows unicode filepaths are encoded in utf16 | ||
| std::string abspath(join_paths(get_cwd(), path)); | ||
| if (!(abspath[0] == '/' && abspath[1] == '/')) { | ||
| abspath = "//?/" + abspath; | ||
| } | ||
| std::wstring wpath(UTF_8::convert_to_utf16(abspath)); | ||
| std::replace(wpath.begin(), wpath.end(), '/', '\\'); | ||
| DWORD rv = GetFullPathNameW(wpath.c_str(), 32767, resolved, NULL); | ||
| DWORD rv = GetFullPathNameW(wpath.c_str(), 32767, &resolved[0], NULL); |
There was a problem hiding this comment.
| DWORD rv = GetFullPathNameW(wpath.c_str(), 32767, &resolved[0], NULL); | |
| DWORD rv = GetFullPathNameW(wpath.c_str(), max_chars, resolved.data(), NULL); |
| // windows unicode filepaths are encoded in utf16 | ||
| std::string abspath(join_paths(get_cwd(), path)); | ||
| if (!(abspath[0] == '/' && abspath[1] == '/')) { | ||
| abspath = "//?/" + abspath; | ||
| } | ||
| std::wstring wpath(UTF_8::convert_to_utf16(abspath)); | ||
| std::replace(wpath.begin(), wpath.end(), '/', '\\'); | ||
| DWORD rv = GetFullPathNameW(wpath.c_str(), 32767, resolved, NULL); | ||
| DWORD rv = GetFullPathNameW(wpath.c_str(), 32767, &resolved[0], NULL); |
There was a problem hiding this comment.
| DWORD rv = GetFullPathNameW(wpath.c_str(), 32767, &resolved[0], NULL); | |
| DWORD rv = GetFullPathNameW(wpath.c_str(), max_chars, resolved.data(), NULL); |
There was a problem hiding this comment.
This shouldn't be necessary, since GetFullPathNameW will fill the buffer anyway!
There was a problem hiding this comment.
This suggestion is to avoid repeating 32767 and to use .data() instead of &[0]
| if (rv > 32767) throw Exception::OperationError("Path is too long"); | ||
| if (rv == 0) throw Exception::OperationError("Path could not be resolved"); | ||
| HANDLE hFile = CreateFileW(resolved, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); | ||
| HANDLE hFile = CreateFileW(&resolved[0], GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); |
There was a problem hiding this comment.
| HANDLE hFile = CreateFileW(&resolved[0], GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); | |
| HANDLE hFile = CreateFileW(resolved.data(), GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); |
| if (rv > 32767) throw Exception::OperationError("Path is too long"); | ||
| if (rv == 0) throw Exception::OperationError("Path could not be resolved"); | ||
| DWORD dwAttrib = GetFileAttributesW(resolved); | ||
| DWORD dwAttrib = GetFileAttributesW(&resolved[0]); |
There was a problem hiding this comment.
| DWORD dwAttrib = GetFileAttributesW(&resolved[0]); | |
| DWORD dwAttrib = GetFileAttributesW(resolved.data()); |
| @@ -437,18 +437,18 @@ namespace Sass { | |||
| #ifdef _WIN32 | |||
| BYTE* pBuffer; | |||
| DWORD dwBytes; | |||
| wchar_t resolved[32768]; | |||
| std::vector<wchar_t> resolved(32768); | |||
There was a problem hiding this comment.
| std::vector<wchar_t> resolved(32768); | |
| const std::size_t max_chars = 32767; | |
| std::vector<wchar_t> resolved(max_chars + 1, 0); |
|
@glebm care to create a PR with your suggested changes!? |
There is nothing wrong per se in having 32KB on stack. We could add an option to have Did this cause real-life crashes of libsass due to running out of stack? Even Alpine Linux lets us having more stack ... (but this code does not apply to Linux). |
|
The memory payload is actually 64k, since the allocated array is of |
No description provided.