<Alexander G. Riccio> via llvm-dev
2016-Apr-18 21:55 UTC
[llvm-dev] Redundant Twine->StringRef->Twine conversions in llvm::sys::fs::make_absolute?
llvm::sys::fs::make_absolute converts its first parameter (const Twine
¤t_directory) to StringRef p(path.data(), path.size()), and then
passes that StringRef to several functions (path::has_root_directory,
path::has_root_name, and path::append) that accept Twines as parameters.
Since llvm::StringRef can implicitly convert to an llvm::Twine, p converts
to a bunch of Twine temporaries.
In a few places, namely path::has_root_directory & path::has_root_name,
that temporary Twine is again converted to a StringRef:
bool has_root_directory(const Twine &path) {
SmallString<128> path_storage;
StringRef p = path.toStringRef(path_storage);
return !root_directory(p).empty();
}
Is there some reason for this? If not, I'll write a patch to delay the
StringRef
p(path.data(), path.size()) construction until it's actually needed (calls
to path::root_name(p) & path::relative_path(p)).
Sincerely,
Alexander Riccio
--
"Change the world or go home."
about.me/ariccio
<http://about.me/ariccio>
If left to my own devices, I will build more.
⁂
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160418/8762dc8b/attachment.html>
Yaron Keren via llvm-dev
2016-Apr-18 22:41 UTC
[llvm-dev] Redundant Twine->StringRef->Twine conversions in llvm::sys::fs::make_absolute?
Are you talking about the local (static) function make_absolute, not the llvm::sys::fs::make_absolute ? Anyhow, StringRef p() is not constructed from the Twine current_directory but from the SmallVector &path which is the relative path to being made absolute, .This StringRef(SmallVector) construction should be zero-cost. The Twine current_directory is constructed only later, if use_current_directory. It may still be possible to optimize this code path but first check how much the use_current_directory=true is code path used vs the use_current_directory=false code path. 2016-04-19 0:55 GMT+03:00 <Alexander G. Riccio> via llvm-dev < llvm-dev at lists.llvm.org>:> llvm::sys::fs::make_absolute converts its first parameter (const Twine > ¤t_directory) to StringRef p(path.data(), path.size()), and then > passes that StringRef to several functions (path::has_root_directory, > path::has_root_name, and path::append) that accept Twines as parameters. > Since llvm::StringRef can implicitly convert to an llvm::Twine, p > converts to a bunch of Twine temporaries. > > In a few places, namely path::has_root_directory & path::has_root_name, > that temporary Twine is again converted to a StringRef: > > bool has_root_directory(const Twine &path) { > SmallString<128> path_storage; > StringRef p = path.toStringRef(path_storage); > > return !root_directory(p).empty(); > } > > Is there some reason for this? If not, I'll write a patch to delay the StringRef > p(path.data(), path.size()) construction until it's actually needed > (calls to path::root_name(p) & path::relative_path(p)). > > Sincerely, > Alexander Riccio > -- > "Change the world or go home." > about.me/ariccio > > <http://about.me/ariccio> > If left to my own devices, I will build more. > ⁂ > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160419/8056949a/attachment.html>
<Alexander G. Riccio> via llvm-dev
2016-Apr-19 01:02 UTC
[llvm-dev] Redundant Twine->StringRef->Twine conversions in llvm::sys::fs::make_absolute?
> > Are you talking about the local (static) function make_absolute, not the > llvm::sys::fs::make_absolute ? >I'm referring to the static function... the funny thing is that the static function is actually defined in the llvm::sys::fs namespace. Anyhow, StringRef p() is not constructed from the Twine current_directory> but from the SmallVector &path which is the relative path to being made > absolute, >Oh, duh. I'm not sure how I missed that while typing my email. That kinda moots the whole point of writing a patch. Nevermind. Sincerely, Alexander Riccio -- "Change the world or go home." about.me/ariccio <http://about.me/ariccio> If left to my own devices, I will build more. ⁂ On Mon, Apr 18, 2016 at 6:41 PM, Yaron Keren <yaron.keren at gmail.com> wrote:> Are you talking about the local (static) function make_absolute, not the > llvm::sys::fs::make_absolute ? > > Anyhow, StringRef p() is not constructed from the Twine current_directory > but from the SmallVector &path which is the relative path to being made > absolute, .This StringRef(SmallVector) construction should be zero-cost. > > The Twine current_directory is constructed only later, > if use_current_directory. It may still be possible to optimize this code > path but first check how much the use_current_directory=true is code path > used vs the use_current_directory=false code path. > > > > 2016-04-19 0:55 GMT+03:00 <Alexander G. Riccio> via llvm-dev < > llvm-dev at lists.llvm.org>: > >> llvm::sys::fs::make_absolute converts its first parameter (const Twine >> ¤t_directory) to StringRef p(path.data(), path.size()), and then >> passes that StringRef to several functions (path::has_root_directory, >> path::has_root_name, and path::append) that accept Twines as parameters. >> Since llvm::StringRef can implicitly convert to an llvm::Twine, p >> converts to a bunch of Twine temporaries. >> >> In a few places, namely path::has_root_directory & path::has_root_name, >> that temporary Twine is again converted to a StringRef: >> >> bool has_root_directory(const Twine &path) { >> SmallString<128> path_storage; >> StringRef p = path.toStringRef(path_storage); >> >> return !root_directory(p).empty(); >> } >> >> Is there some reason for this? If not, I'll write a patch to delay the StringRef >> p(path.data(), path.size()) construction until it's actually needed >> (calls to path::root_name(p) & path::relative_path(p)). >> >> Sincerely, >> Alexander Riccio >> -- >> "Change the world or go home." >> about.me/ariccio >> >> <http://about.me/ariccio> >> If left to my own devices, I will build more. >> ⁂ >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160418/131009f5/attachment-0001.html>