Re: [PATCH] Micro-optimization to avoid creating temporary path

[Jonathan Wakely <jwakely at redhat dot com>]

On 20/12/18 04:06 -0500, Tim Song wrote:
> On Thu, Dec 20, 2018 at 1:12 AM François Dumont <frs.dumont@gmail.com> wrote:
> > On 12/18/18 5:38 PM, Jonathan Wakely wrote:
> > > On 18/12/18 15:52 +0000, Jonathan Wakely wrote:
> > >> Now that path::operator/=(basic_string_view<value_type>) works directly
> > >> from the string argument, instead of constructing a temporary path from
> > >> the string, it's potentially more efficient to do 'path(x) /= s' instead
> > >> of 'x / s'. This changes the only relevant place in the library.
> > >>
> > >>     * src/filesystem/std-dir.cc (filesystem::_Dir::advance): Append
> > >>     string to lvalue to avoid creating temporary path.
> > >
> > > This is only an optimization if it doesn't introduce a new copy! Fixed
> > > by the attached patch.
> > >
> > > Tested x86_64-linux, committed to trunk.
> > >
> > >
> > Isn't it just equivalent to the original code:
> >
> > -       entry = fs::directory_entry{path / entp->d_name, get_file_type(*entp)};
> >
> > The temporary un-named instance was already moved, no ?
>
> The saving is from avoiding converting entp->d_name to a path
> (operator/ is only defined for when both sides are paths).

Right. The original code called:

path operator/(const path&, const path&)

which means it's equivalent to: (p / path(d_name)).

The new code is: auto p2 = p; p2 /= name; std::move(p2);

This uses path::operator/=(const char*) which doesn't need to create
the temporary path(d_name).

It won't matter much for the new std::string and filenames that fit in
the SSO buffer, but for longer filenames it saves an allocation.