llvm dev - Apr 2016 - Redundant Twine->StringRef->Twine conversions in llvm::sys::fs::make_absolute?

llvm::sys::fs::make_absolute converts its first parameter (const Twine
&current_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>

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
> &current_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>

>
> 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
>> &current_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>