Skip to content

src: refactor SplitString in util - #48491

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
anonrig:refactor-split-string
Jun 22, 2023
Merged

src: refactor SplitString in util#48491
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
anonrig:refactor-split-string

Conversation

@anonrig

@anonriganonrig commented Jun 18, 2023

Copy link
Copy Markdown
Member

See quick-bench.com

Screenshot 2023-06-18 at 12 48 39 PM
benchmark code
#include<string_view>
#include<algorithm>// uses string::find_first_of
std::vector<std::string>
split(const std::string& str, const std::string& delims = "")
{
std::vector<std::string> output;
//output.reserve(str.length() / 4);size_t first = 0;
while (first < str.size())
{
constauto second = str.find_first_of(delims, first);
if (first != second)
{
output.emplace_back(str.substr(first, second-first));
}
if (second == std::string::npos)
break;
first = second + 1;
}
return output;
}
std::vector<std::string>
splitStd(const std::string& str, const std::string& delims = "")
{
std::vector<std::string> output;
auto first = std::cbegin(str);
while (first != std::cend(str))
{
constauto second = std::find_first_of(first, std::cend(str),
std::cbegin(delims), std::cend(delims));
if (first != second)
output.emplace_back(first, second);
if (second == std::cend(str))
break;
first = std::next(second);
}
return output;
}
std::vector<std::string> splitPtr(const std::string& str, const std::string& delims = "")
{
std::vector<std::string> output;
//	output.reserve(str.size() / 2);for (auto first = str.data(), second = str.data(), last = first + str.size(); second != last && first != last; first = second + 1) {
second = std::find_first_of(first, last, std::cbegin(delims), std::cend(delims));
if (first != second)
output.emplace_back(first, second);
}
return output;
}
std::vector<std::string_view>
splitSV(std::string_view strv, std::string_view delims = "")
{
std::vector<std::string_view> output;
size_t first = 0;
while (first < strv.size())
{
constauto second = strv.find_first_of(delims, first);
if (first != second)
output.emplace_back(strv.substr(first, second-first));
if (second == std::string_view::npos)
break;
first = second + 1;
}
return output;
}
std::vector<std::string_view>
splitSVStd(std::string_view strv, std::string_view delims = "")
{
std::vector<std::string_view> output;
//output.reserve(strv.length() / 4);auto first = strv.begin();
while (first != strv.end())
{
constauto second = std::find_first_of(first, std::cend(strv),
std::cbegin(delims), std::cend(delims));
//std::cout << first << ", " << second << '\n';if (first != second)
{
output.emplace_back(strv.substr(std::distance(strv.begin(), first), std::distance(first, second)));
}
if (second == strv.end())
break;
first = std::next(second);
}
return output;
}
std::vector<std::string_view> splitSVPtr(std::string_view str, std::string_view delims = "")
{
std::vector<std::string_view> output;
//output.reserve(str.size() / 2);for (auto first = str.data(), second = str.data(), last = first + str.size(); second != last && first != last; first = second + 1) {
second = std::find_first_of(first, last, std::cbegin(delims), std::cend(delims));
if (first != second)
output.emplace_back(first, second - first);
}
return output;
}
const std::string_view LoremIpsumStrv{ "Lorem ipsum dolor sit amet, consectetur adipiscing elit,""sed do eiusmod tempor incididuntsuperlongwordsuper ut labore et dolore magna aliqua. Ut enim ad minim veniam, ""quis nostrud exercitation ullamco laboris nisi ut aliquipsuperlongword ex ea commodo consequat. Duis aute""irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.""Excepteur sint occaecat cupidatatsuperlongword non proident, sunt in culpa qui officia deserunt mollit anim id est laborum." };
staticvoidStringSplit(benchmark::State& state) {
std::string str { LoremIpsumStrv };
// Code inside this loop is measured repeatedlyfor (auto _ : state) {
auto v = split(str);
benchmark::DoNotOptimize(v);
}
}
// Register the function as a benchmarkBENCHMARK(StringSplit);
staticvoidStringSplitStd(benchmark::State& state) {
std::string str { LoremIpsumStrv };
// Code inside this loop is measured repeatedlyfor (auto _ : state) {
auto v = splitStd(str);
benchmark::DoNotOptimize(v);
}
}
// Register the function as a benchmarkBENCHMARK(StringSplitStd);
staticvoidStringSplitPtr(benchmark::State& state) {
std::string str { LoremIpsumStrv };
// Code inside this loop is measured repeatedlyfor (auto _ : state) {
auto v = splitPtr(str);
benchmark::DoNotOptimize(v);
}
}
// Register the function as a benchmarkBENCHMARK(StringSplitPtr);
staticvoidStringViewSplit(benchmark::State& state) {
for (auto _ : state) {
auto v = splitSV(LoremIpsumStrv);
benchmark::DoNotOptimize(v);
}
}
BENCHMARK(StringViewSplit);
staticvoidStringViewSplitStd(benchmark::State& state) {
for (auto _ : state) {
auto v = splitSVStd(LoremIpsumStrv);
benchmark::DoNotOptimize(v);
}
}
BENCHMARK(StringViewSplitStd);
staticvoidStringViewSplitPtr(benchmark::State& state) {
for (auto _ : state) {
auto v = splitSVPtr(LoremIpsumStrv);
benchmark::DoNotOptimize(v);
}
}
BENCHMARK(StringViewSplitPtr);

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 18, 2023
@anonriganonrig added the performance Issues and PRs related to the performance of Node.js. label Jun 18, 2023
@targos

Copy link
Copy Markdown
Member

Please at least post the benchmark results in the PR. Your link may become dead in the future

Comment threadsrc/permission/fs_permission.cc Outdated
@anonriganonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 21, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonriganonrig removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 22, 2023
Comment threadsrc/node_options.cc Outdated
Comment threadsrc/node_options.cc Outdated
Comment threadsrc/node_options.cc Outdated
Comment threadsrc/node_options.cc Outdated
Comment threadsrc/node_options.cc Outdated
Comment threadsrc/node_v8_platform-inl.h Outdated
Comment threadsrc/permission/fs_permission.cc Outdated
@anonrig
anonrigforce-pushed the refactor-split-string branch from 8304083 to 27d1a87CompareJune 22, 2023 19:27
@anonrig

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/cpp-reviewers

@anonrig
anonrigforce-pushed the refactor-split-string branch from 27d1a87 to 63f8f96CompareJune 22, 2023 19:47
@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonriganonrig added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 22, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 640a791 into nodejs:mainJun 22, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 640a791

RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
PR-URL: #48491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Jul 3, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 10, 2023
PR-URL: #48491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@ruyadornoruyadorno mentioned this pull request Sep 10, 2023
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
PR-URL: #48491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
tniessen added a commit to tniessen/node that referenced this pull request Nov 17, 2023
The use of string_view and subsequent copying to a string was supposed
to be a minor optimization in 640a791, however, since 413c16e, no
string splitting occurs anymore. Therefore, we can simply pass around
some references instead of using string_view or copying strings.
Refs: nodejs#48491
Refs: nodejs#49047
nodejs-github-bot pushed a commit that referenced this pull request Nov 19, 2023
The use of string_view and subsequent copying to a string was supposed
to be a minor optimization in 640a791, however, since 413c16e, no
string splitting occurs anymore. Therefore, we can simply pass around
some references instead of using string_view or copying strings.
Refs: #48491
Refs: #49047
PR-URL: #50662
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Nov 23, 2023
The use of string_view and subsequent copying to a string was supposed
to be a minor optimization in 640a7918, however, since 413c16e, no
string splitting occurs anymore. Therefore, we can simply pass around
some references instead of using string_view or copying strings.
Refs: #48491
Refs: #49047
PR-URL: #50662
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
The use of string_view and subsequent copying to a string was supposed
to be a minor optimization in 640a791, however, since 413c16e, no
string splitting occurs anymore. Therefore, we can simply pass around
some references instead of using string_view or copying strings.
Refs: nodejs#48491
Refs: nodejs#49047
PR-URL: nodejs#50662
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Nov 29, 2023
The use of string_view and subsequent copying to a string was supposed
to be a minor optimization in 640a791, however, since 413c16e, no
string splitting occurs anymore. Therefore, we can simply pass around
some references instead of using string_view or copying strings.
Refs: #48491
Refs: #49047
PR-URL: #50662
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
The use of string_view and subsequent copying to a string was supposed
to be a minor optimization in 640a791, however, since 413c16e, no
string splitting occurs anymore. Therefore, we can simply pass around
some references instead of using string_view or copying strings.
Refs: #48491
Refs: #49047
PR-URL: #50662
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
The use of string_view and subsequent copying to a string was supposed
to be a minor optimization in 640a791, however, since 413c16e, no
string splitting occurs anymore. Therefore, we can simply pass around
some references instead of using string_view or copying strings.
Refs: #48491
Refs: #49047
PR-URL: #50662
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@anonrig@nodejs-github-bot@targos@Qard@lemire@jasnell@RafaelGSS