From 2fc506d1bbaebe0bd8923b83d038adb11e32c61e Mon Sep 17 00:00:00 2001 From: q66 Date: Sat, 13 May 2017 16:19:05 +0200 Subject: [PATCH] fewer temporary allocations in subprocess --- ostd/process.hh | 27 +++++------ src/posix/process.cc | 53 +++++++++++++++++---- src/win32/process.cc | 108 ++++++++++++++++++++++++++----------------- 3 files changed, 122 insertions(+), 66 deletions(-) diff --git a/ostd/process.hh b/ostd/process.hh index f24ff84..a163a27 100644 --- a/ostd/process.hh +++ b/ostd/process.hh @@ -383,7 +383,7 @@ private: template void open_full(string_range cmd, InputRange args, bool use_path) { static_assert( - std::is_constructible_v>, + std::is_constructible_v>, "The arguments must be strings" ); @@ -391,25 +391,20 @@ private: throw subprocess_error{"another child process already running"}; } - std::vector argv; - for (; !args.empty(); args.pop_front()) { - argv.emplace_back(args.front()); - } - if (argv.empty()) { - throw subprocess_error{"no arguments given"}; - } - if (cmd.empty()) { - cmd = argv[0]; - if (cmd.empty()) { - throw subprocess_error{"no command given"}; + open_impl(use_path, cmd, [](string_range &arg, void *data) { + InputRange &argr = *static_cast(data); + if (argr.empty()) { + return false; } - } - open_impl(std::string{cmd}, argv, use_path); + arg = string_range{argr.front()}; + argr.pop_front(); + return true; + }, &args); } void open_impl( - std::string const &cmd, std::vector const &args, - bool use_path + bool use_path, string_range cmd, + bool (*func)(string_range &, void *), void *data ); void reset(); diff --git a/src/posix/process.cc b/src/posix/process.cc index 6e2a665..34655c0 100644 --- a/src/posix/process.cc +++ b/src/posix/process.cc @@ -132,18 +132,55 @@ struct pipe { }; OSTD_EXPORT void subprocess::open_impl( - std::string const &cmd, std::vector const &args, - bool use_path + bool use_path, string_range cmd, + bool (*func)(string_range &, void *), void *datap ) { if (use_in == subprocess_stream::STDOUT) { throw subprocess_error{"could not redirect stdin to stdout"}; } - auto argp = std::make_unique(args.size() + 1); - for (std::size_t i = 0; i < args.size(); ++i) { - argp[i] = const_cast(args[i].data()); + struct argpv { + char *cmd = nullptr; + std::vector vec; + ~argpv() { + auto vs = vec.size(); + if (!vs) { + return; + } + if (cmd && (cmd != vec[0])) { + delete[] cmd; + } + for (std::size_t i = 0; i < (vs - 1); ++i) { + delete[] vec[i]; + } + } + } argp; + + for (string_range r; func(r, datap);) { + auto sz = r.size(); + char *str = new char[sz + 1]; + string_range::traits_type::copy(str, r.data(), sz)[sz] = '\0'; + argp.vec.push_back(str); } - argp[args.size()] = nullptr; + + if (argp.vec.empty()) { + throw subprocess_error{"no arguments given"}; + } + + if (cmd.empty()) { + argp.cmd = argp.vec[0]; + if (!*argp.cmd) { + throw subprocess_error{"no command given"}; + } + } else { + auto sz = cmd.size(); + char *str = new char[sz + 1]; + string_range::traits_type::copy(str, cmd.data(), sz)[sz] = '\0'; + argp.cmd = str; + } + + /* terminate */ + argp.vec.push_back(nullptr); /* fd_errno used to detect if exec failed */ pipe fd_errno, fd_stdin, fd_stdout, fd_stderr; @@ -189,9 +226,9 @@ OSTD_EXPORT void subprocess::open_impl( } } if (use_path) { - execvp(cmd.data(), argp.get()); + execvp(argp.cmd, argp.vec.data()); } else { - execv(cmd.data(), argp.get()); + execv(argp.cmd, argp.vec.data()); } /* exec has returned, so error has occured */ fd_errno.write_errno(); diff --git a/src/win32/process.cc b/src/win32/process.cc index 9973f3c..f46ac55 100644 --- a/src/win32/process.cc +++ b/src/win32/process.cc @@ -21,6 +21,7 @@ #include #include +#include "ostd/algprithm.hh" #include "ostd/process.hh" #include "ostd/format.hh" @@ -237,47 +238,93 @@ static std::wstring resolve_file(wchar_t const *cmd) { * * we need to replicate this awful behavior here, hence the extra code */ -static std::string concat_args(std::vector const &args) { +static std::unique_ptr concat_args( + string_range cmd, bool (*func)(string_range &, void *), void *datap, + std::wstring &cmdpath +) { std::string ret; - for (auto &s: args) { + + string_range p; + if (!func(p, datap)) { + throw subprocess_error{"no arguments given"}; + } + if (!cmd.size()) { + cmd = p; + if (!cmd.size()) { + throw subprocess_error{"no command given"}; + } + } + + /* convert and optionally resolve PATH and other lookup locations */ + { + std::unique_ptr wcmd{new wchar_t[cmd.size() + 1]}; + auto req = MultiByteToWideChar( + CP_UTF8, 0, cmd.data(), cmd.size(), wcmd.get(), cmd.size() + 1 + ); + if (!req) { + throw subprocess_error{"unicode conversion failed"}; + } + wcmd.get()[req] = '\0'; + if (!use_path) { + cmdpath = wcmd.get(); + } else { + cmdpath = resolve_file(wcmd.get()); + } + } + + /* concat the args */ + for (bool has = true; has; has = func(p, datap)) { if (!ret.empty()) { ret += ' '; } ret += '\"'; - for (char const *sp = s.data();;) { - char const *p = strpbrk(sp, "\"\\"); - if (!p) { - ret += sp; + for (;;) { + auto found = ostd::find_one_of(p, string_range{"\"\\"}); + if (found.empty()) { + ret.append(p.iter_begin(), p.iter_end()); break; } - ret.append(sp, p); - if (*p == '\"') { + auto sl = p.slice(0, p.size() - found.size()); + ret.append(sl.iter_begin(), sl.iter_end()); + if (found.front() == '\"') { /* not preceded by \, so it's safe */ ret += "\\\""; - ++p; + found.pop_front(); } else { /* handle any sequence of \ optionally followed by a " */ - char const *op = p; - while (*p == '\\') { - ++p; + std::size_t nsl = 0; + while (found.front() == '\\') { + ++nsl; + found.pop_front(); } - if (*p == '\"') { + if (!found.empty() && (found.front() == '\"')) { /* double all the backslashes plus one for the " */ - ret.append((p - op) * 2 + 1, '\\'); + ret.append(nsl * 2 + 1, '\\'); ret += '\"'; } else { - ret.append(p - op, '\\'); + /* double if the backslashes were at the end of the arg */ + ret.append(nsl * (found.empty() + 1), '\\'); } } - sp = p; + p = found; } ret += '\"'; } - return ret; + + /* convert to UTF-16 */ + std::unique_ptr cmdline{new wchar_t[ret.size() + 1]}; + if (!MultiByteToWideChar( + CP_UTF8, 0, ret.data(), ret.size() + 1, cmdline.get(), ret.size() + 1 + )) { + throw subprocess_error{"unicode conversion failed"}; + } + + return cmdline; } OSTD_EXPORT void subprocess::open_impl( - std::string const &cmd, std::vector const &args, bool use_path + bool use_path, string_range cmd, + bool (*func)(string_range &, void *), void *datap ) { if (use_in == subprocess_stream::STDOUT) { throw subprocess_error{"could not redirect stdin to stdout"}; @@ -369,30 +416,7 @@ OSTD_EXPORT void subprocess::open_impl( si.dwFlags |= STARTF_USESTDHANDLES; std::wstring cmdpath; - /* convert and optionally resolve PATH and other lookup locations */ - { - std::unique_ptr wcmd{new wchar_t[cmd.size() + 1]}; - if (!MultiByteToWideChar( - CP_UTF8, 0, cmd.data(), cmd.size() + 1, wcmd.get(), cmd.size() + 1 - )) { - throw subprocess_error{"unicode conversion failed"}; - } - if (!use_path) { - cmdpath = wcmd.get(); - } else { - cmdpath = resolve_file(wcmd.get()); - } - } - - /* cmdline gets an ordinary conversion... */ - auto astr = concat_args(args); - - std::unique_ptr cmdline{new wchar_t[astr.size() + 1]}; - if (!MultiByteToWideChar( - CP_UTF8, 0, astr.data(), astr.size() + 1, cmdline.get(), astr.size() + 1 - )) { - throw subprocess_error{"unicode conversion failed"}; - } + auto cmdline = concat_args(cmd, func, datap, cmdpath); /* owned by CreateProcess, do not close explicitly */ pipe_in.p_r = nullptr;