Commit d8a7b7fc authored by Igor Kushnir's avatar Igor Kushnir
Browse files

Path::segments(): return a const reference instead of a copy

Don't copy Path segments unnecessarily in qHash(const Path&). This
function may be called often in performance-critical code.

This commit speeds up BenchQuickOpen::benchProjectFileFilter_setFilter
by about 10% when the filter is not empty. matchPathFilter() calls
Path::segments() twice, is called by PathFilter::setFilter() in a loop.

I have examined all usages of Path::segments() in KDevelop code and
found no possibility of a regression because of this return type change.
There are no usages of Path::segments() in maintained KDevelop plugins.

In many usages copying the QVector is unnecessary and it doesn't occur
with this change, which improves performance. Copying a QVector could be
the bottleneck in some cases. One such case was a lambda in
ProjectFileDataProvider::projectClosing(), which called this function on
each file's projectPath (in an optimization now discarded in favor of an
almost equivalent Path::operator== change in the previous commit).

Arguments for returning the QVector by value:
  * returning objects by value is generally considered safer than
    returning them by reference;
  * returning by value gives more freedom to change the implementation
    of a class without changing its interface;
  * non-template Qt classes conventionally return by value relying on
    copy-on-write to make this relatively cheap.

Arguments for returning the QVector by reference:
  * Returning by reference is not very dangerous. This pattern is not
    uncommon in non-COW C++ code and is used extensively by Qt and std
    container class templates, so developers know what to expect and how
    to use such interfaces safely. If callers of this function store the
    return value in a reference variable for optimization, they are
    responsible for ensuring the safety of such usage. If they simply
    use segments() in an expression or assign its return value to a
    non-reference variable (e.g. with `auto`), then they should be safe
    from reference-related bugs.
  * Path is a class whose main purpose is optimization. Its layout is
    already inline. If it switches to some other class for storing its
    internal data, the ABI change is unavoidable; the not so many usages
    of segments() could be adjusted accordingly rather than become
    silently a lot less efficient.
  * Returning QVector by value not only imposes the cost of copying it,
    but creates a risk of detaching if not used carefully: even when a
    Path is constant, `Path::segments() const` returns a temporary,
    non-const QVector. So the users of this function should take care to
    call only purely const QVector member functions or wrap segments()
    in as_const or assign segments() to a temporary constant variable.
parent b13d377c
......@@ -472,8 +472,7 @@ namespace KDevelop {
uint qHash(const Path& path)
{
KDevHash hash;
const auto pathSegments = path.segments();
for (const QString& segment : pathSegments) {
for (const QString& segment : path.segments()) {
hash << qHash(segment);
}
......
......@@ -280,9 +280,14 @@ public:
QString remotePrefix() const;
/**
* @return an implicitly shared copy of the internal data.
* @return a const reference to the internal data.
*
* @note Returning a reference to rather than a copy of QVector can substantially
* improve performance of a tight loop that calls this function.
*
* TODO: return std::span once we can rely on C++20.
*/
inline QVector<QString> segments() const
inline const QVector<QString>& segments() const
{
return m_data;
}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment