From f9682734b8851c604ad7011bba17795a2b304707 Mon Sep 17 00:00:00 2001 From: Tomasz Konojacki Date: Sun, 29 Oct 2023 20:08:52 +0100 Subject: [PATCH 01/13] : Preallocate memory in path::operator/ Now, in the typical case, it will do at most a single allocation. It's a hot path in my program, and a cursory GitHub search showed that path::operator/ is indeed commonly used. --- stl/inc/filesystem | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index a2abcecd464..e075af8a6b6 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1377,8 +1377,24 @@ namespace filesystem { #endif // ^^^ !_HAS_CXX20 ^^^ _NODISCARD_FRIEND path operator/(const path& _Left, const path& _Right) { // append a pair of paths together - path _Tmp = _Left; + path _Tmp; + const auto _Right_size = _Right._Text.size(); + const auto _Right_first = _Right._Text.data(); + const auto _Right_last = _Right_first + _Right_size; + + // Preallocate memory for the most common case + if (!_Has_drive_letter_prefix(_Right_first, _Right_last) && !_Is_slash(*_Right_first)) { + // !has_root_name(_Right) && !has_root_directory(_Right) + const auto _Left_size = _Left._Text.size(); + const auto _Left_last = _Left._Text.data() + _Left_size; + const bool _Left_has_trailing_slash = _Left_size && _Is_slash(_Left_last[-1]); + + _Tmp._Text.reserve(_Left_size + !_Left_has_trailing_slash + _Right_size); + } + + _Tmp = _Left; _Tmp /= _Right; + return _Tmp; } From 021c180c6d3a0a3845f9d8472d53ddd9dffe2d93 Mon Sep 17 00:00:00 2001 From: Tomasz Konojacki Date: Mon, 30 Oct 2023 16:57:55 +0100 Subject: [PATCH 02/13] don't implicitly convert int to bool --- stl/inc/filesystem | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index e075af8a6b6..2007dcb24f0 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1387,7 +1387,7 @@ namespace filesystem { // !has_root_name(_Right) && !has_root_directory(_Right) const auto _Left_size = _Left._Text.size(); const auto _Left_last = _Left._Text.data() + _Left_size; - const bool _Left_has_trailing_slash = _Left_size && _Is_slash(_Left_last[-1]); + const bool _Left_has_trailing_slash = _Left_size != 0 && _Is_slash(_Left_last[-1]); _Tmp._Text.reserve(_Left_size + !_Left_has_trailing_slash + _Right_size); } From 9b49a55bf388e78c3ed098f0deb05a00dce0ff8a Mon Sep 17 00:00:00 2001 From: Tomasz Konojacki Date: Mon, 30 Oct 2023 21:34:16 +0100 Subject: [PATCH 03/13] use _Resize_and_overwrite --- stl/inc/filesystem | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index 2007dcb24f0..657bd1a6aa7 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1377,24 +1377,38 @@ namespace filesystem { #endif // ^^^ !_HAS_CXX20 ^^^ _NODISCARD_FRIEND path operator/(const path& _Left, const path& _Right) { // append a pair of paths together - path _Tmp; const auto _Right_size = _Right._Text.size(); const auto _Right_first = _Right._Text.data(); const auto _Right_last = _Right_first + _Right_size; - // Preallocate memory for the most common case + // Handle the most common case if (!_Has_drive_letter_prefix(_Right_first, _Right_last) && !_Is_slash(*_Right_first)) { // !has_root_name(_Right) && !has_root_directory(_Right) - const auto _Left_size = _Left._Text.size(); - const auto _Left_last = _Left._Text.data() + _Left_size; - const bool _Left_has_trailing_slash = _Left_size != 0 && _Is_slash(_Left_last[-1]); - - _Tmp._Text.reserve(_Left_size + !_Left_has_trailing_slash + _Right_size); + const auto _Left_size = _Left._Text.size(); + const auto _Left_first = _Left._Text.data(); + const auto _Left_last = _Left._Text.data() + _Left_size; + + // Appending a slash to "X:" would've made it an absolute path + const bool _Left_is_just_drive = _Left_size == 2 && _Is_drive_prefix(_Left_first); + const bool _Is_slash_needed = _Left_size != 0 && !_Left_is_just_drive && !_Is_slash(_Left_last[-1]); + + const auto _Total_size = _Left_size + _Is_slash_needed + _Right_size; + + path _Tmp; + _Tmp._Text._Resize_and_overwrite(_Total_size, [&](wchar_t* _Ptr, size_t _Size) { + _CSTD memcpy(_Ptr, _Left_first, _Left_size * sizeof(wchar_t)); + _Ptr += _Left_size; + if (_Is_slash_needed) { + *_Ptr++ = preferred_separator; + } + _CSTD memcpy(_Ptr, _Right_first, _Right_size * sizeof(wchar_t)); + return _Size; + }); + return _Tmp; } - _Tmp = _Left; + path _Tmp = _Left; _Tmp /= _Right; - return _Tmp; } From 4b089ca52333a6b065fbbc597058df3028d89c82 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 07:27:11 -0800 Subject: [PATCH 04/13] Fuse comments to avoid looking like commented-out code. --- stl/inc/filesystem | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index eb71242e81c..cd215ffc842 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1381,9 +1381,8 @@ namespace filesystem { const auto _Right_first = _Right._Text.data(); const auto _Right_last = _Right_first + _Right_size; - // Handle the most common case + // Handle the most common case: !has_root_name(_Right) && !has_root_directory(_Right) if (!_Has_drive_letter_prefix(_Right_first, _Right_last) && !_Is_slash(*_Right_first)) { - // !has_root_name(_Right) && !has_root_directory(_Right) const auto _Left_size = _Left._Text.size(); const auto _Left_first = _Left._Text.data(); const auto _Left_last = _Left._Text.data() + _Left_size; From 404cd63d6472163e6363774535e8ab4479f27e77 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 07:29:29 -0800 Subject: [PATCH 05/13] Reuse `_Left_first`, consistent with `_Right_MEOW` above. --- stl/inc/filesystem | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index cd215ffc842..afdb5b56782 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1385,7 +1385,7 @@ namespace filesystem { if (!_Has_drive_letter_prefix(_Right_first, _Right_last) && !_Is_slash(*_Right_first)) { const auto _Left_size = _Left._Text.size(); const auto _Left_first = _Left._Text.data(); - const auto _Left_last = _Left._Text.data() + _Left_size; + const auto _Left_last = _Left_first + _Left_size; // Appending a slash to "X:" would've made it an absolute path const bool _Left_is_just_drive = _Left_size == 2 && _Is_drive_prefix(_Left_first); From f79439aab743efd06c025ea100ae468f06cfd33a Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 07:34:04 -0800 Subject: [PATCH 06/13] Capture by copy, which might help alias analysis. --- stl/inc/filesystem | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index afdb5b56782..3e6c7280d97 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1394,7 +1394,7 @@ namespace filesystem { const auto _Total_size = _Left_size + _Is_slash_needed + _Right_size; path _Tmp; - _Tmp._Text._Resize_and_overwrite(_Total_size, [&](wchar_t* _Ptr, size_t _Size) { + _Tmp._Text._Resize_and_overwrite(_Total_size, [=](wchar_t* _Ptr, size_t _Size) { _CSTD memcpy(_Ptr, _Left_first, _Left_size * sizeof(wchar_t)); _Ptr += _Left_size; if (_Is_slash_needed) { From abacb4f65f5a091deb5167d4816b6290f3550442 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 08:15:33 -0800 Subject: [PATCH 07/13] Check `_Right_size != 0` before inspecting `*_Right_first`. Co-authored-by: Casey Carter --- stl/inc/filesystem | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index 3e6c7280d97..455adafc7f0 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1382,7 +1382,7 @@ namespace filesystem { const auto _Right_last = _Right_first + _Right_size; // Handle the most common case: !has_root_name(_Right) && !has_root_directory(_Right) - if (!_Has_drive_letter_prefix(_Right_first, _Right_last) && !_Is_slash(*_Right_first)) { + if (_Right_size != 0 && !_Has_drive_letter_prefix(_Right_first, _Right_last) && !_Is_slash(*_Right_first)) { const auto _Left_size = _Left._Text.size(); const auto _Left_first = _Left._Text.data(); const auto _Left_last = _Left_first + _Left_size; From 1f0a307706d4f8557dd667efd8e91e41d85a9557 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 08:25:13 -0800 Subject: [PATCH 08/13] Grammar: "would've made" => "would make" We're talking about something in the future that we want to avoid doing, instead of something in the past that we avoided doing. --- stl/inc/filesystem | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index 455adafc7f0..2ff9bfb97fc 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1387,7 +1387,7 @@ namespace filesystem { const auto _Left_first = _Left._Text.data(); const auto _Left_last = _Left_first + _Left_size; - // Appending a slash to "X:" would've made it an absolute path + // Appending a slash to "X:" would make it an absolute path const bool _Left_is_just_drive = _Left_size == 2 && _Is_drive_prefix(_Left_first); const bool _Is_slash_needed = _Left_size != 0 && !_Left_is_just_drive && !_Is_slash(_Left_last[-1]); From a488f8dc1da19e3a9791fc55a3d2d857809c58e2 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 08:27:26 -0800 Subject: [PATCH 09/13] The `_Size` parameter can be `const`. --- stl/inc/filesystem | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index 2ff9bfb97fc..b4e27b1e3de 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1394,7 +1394,7 @@ namespace filesystem { const auto _Total_size = _Left_size + _Is_slash_needed + _Right_size; path _Tmp; - _Tmp._Text._Resize_and_overwrite(_Total_size, [=](wchar_t* _Ptr, size_t _Size) { + _Tmp._Text._Resize_and_overwrite(_Total_size, [=](wchar_t* _Ptr, const size_t _Size) { _CSTD memcpy(_Ptr, _Left_first, _Left_size * sizeof(wchar_t)); _Ptr += _Left_size; if (_Is_slash_needed) { From 23cf2c2963c74b4368bbe997518246ce5820fe8a Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 08:35:50 -0800 Subject: [PATCH 10/13] Style: `static_cast` before adding `bool`. --- stl/inc/filesystem | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index b4e27b1e3de..a08e435d4cc 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -1391,7 +1391,7 @@ namespace filesystem { const bool _Left_is_just_drive = _Left_size == 2 && _Is_drive_prefix(_Left_first); const bool _Is_slash_needed = _Left_size != 0 && !_Left_is_just_drive && !_Is_slash(_Left_last[-1]); - const auto _Total_size = _Left_size + _Is_slash_needed + _Right_size; + const auto _Total_size = _Left_size + static_cast(_Is_slash_needed) + _Right_size; path _Tmp; _Tmp._Text._Resize_and_overwrite(_Total_size, [=](wchar_t* _Ptr, const size_t _Size) { From 6d384e71be874054fe123e377a2cb81aaa5de0ea Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 08:55:20 -0800 Subject: [PATCH 11/13] Test both `operator/=` and `operator/`. --- tests/std/tests/P0218R1_filesystem/test.cpp | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index d3ea57bbbc8..57657defd3f 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -568,13 +568,23 @@ constexpr slash_test_case slashTestCases[] = { bool run_slash_test_case(const slash_test_case& testCase) { path p(testCase.a); p /= testCase.b; - if (p.native() == testCase.expected) { - return true; + + if (p.native() != testCase.expected) { + wcerr << L"With operator/=, expected " << testCase.a << L" / " << testCase.b << L" to be " << testCase.expected + << L" but it was " << p.native() << L"\n"; + return false; } - wcerr << L"Expected " << testCase.a << L" / " << testCase.b << L" to be " << testCase.expected << L" but it was " - << p.native() << L"\n"; - return false; + // Also test operator/, which was optimized by GH-4136. + p = path{testCase.a} / path{testCase.b}; + + if (p.native() != testCase.expected) { + wcerr << L"With operator/, expected " << testCase.a << L" / " << testCase.b << L" to be " << testCase.expected + << L" but it was " << p.native() << L"\n"; + return false; + } + + return true; } void test_iterators() { From f4637f415bf82a917ef9be8c020c81903d293673 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 09:24:49 -0800 Subject: [PATCH 12/13] Test the both-empty case. This doesn't exercise the optimization, it's just a good idea. --- tests/std/tests/P0218R1_filesystem/test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index 57657defd3f..650e91b47ab 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -544,6 +544,7 @@ struct slash_test_case { }; constexpr slash_test_case slashTestCases[] = { + {L""sv, L""sv, L""sv}, {L"relative"sv, L"other"sv, LR"(relative\other)"sv}, {L"//server"sv, L"share"sv, LR"(//server\share)"sv}, {L"//server/"sv, L"share"sv, LR"(//server/share)"sv}, From 381580fa86daf9d07ed300a21362dffc47065d8e Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 25 Jan 2024 09:25:33 -0800 Subject: [PATCH 13/13] Add a test case to exercise the optimization. With this, I believe all codepaths are exercised. --- tests/std/tests/P0218R1_filesystem/test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index 650e91b47ab..e2d66ba01b2 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -556,6 +556,7 @@ constexpr slash_test_case slashTestCases[] = { {L""sv, L"cat"sv, L"cat"sv}, {L"./"sv, L"cat"sv, L"./cat"sv}, // original test case catching a bug in the above {L"c:"sv, L""sv, L"c:"sv}, + {L"c:"sv, L"dog"sv, L"c:dog"sv}, {L"c:cat"sv, L"/dog"sv, L"c:/dog"sv}, {L"c:/cat"sv, L"/dog"sv, L"c:/dog"sv}, {L"c:cat"sv, L"c:dog"sv, LR"(c:cat\dog)"sv},