Differential D13820

Use QHash::value() in EffectWindowImpl::data()
ClosedPublic
Actions

Authored by zzag on Jul 1 2018, 7:41 AM.

Details

Reviewers
davidedmundson
Group Reviewers
KWin
Commits
R108:6f173fc609c9: Use QHash::value() in EffectWindowImpl::data()
Summary

The role hash key is hashed twice:

  • first, when calling contains method;
  • second, when using operator[].

We can do better by using QHash::value.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zzag created this revision.Jul 1 2018, 7:41 AM
Restricted Application added a project: KWin. · View Herald TranscriptJul 1 2018, 7:41 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Jul 1 2018, 7:41 AM
davidedmundson accepted this revision.Jul 1 2018, 7:46 AM
davidedmundson added a subscriber: davidedmundson.

Const find/ constEnd should work here.

This revision is now accepted and ready to land.Jul 1 2018, 7:46 AM
pino added a subscriber: pino.Jul 1 2018, 7:50 AM

What about something simpler, e.g.:

return dataMap.value(role, QVariant());
zzag added a comment.Jul 1 2018, 7:54 AM
In D13820#285496, @pino wrote:

What about something simpler, e.g.:

return dataMap.value(role, QVariant());

I don't like allocating space for QVariant even when we don't need it.

zzag added a comment.Jul 1 2018, 7:55 AM
In D13820#285492, @davidedmundson wrote:

Const find/ constEnd should work here.

Yeah, we make a copy so that should be fine. Thanks.

zzag updated this revision to Diff 36984.Jul 1 2018, 7:55 AM

constFind/constEnd

pino added a comment.EditedJul 1 2018, 8:04 AM
In D13820#285498, @zzag wrote:
In D13820#285496, @pino wrote:

What about something simpler, e.g.:

return dataMap.value(role, QVariant());

I don't like allocating space for QVariant even when we don't need it.

Hardly an issue... Also, if you talk about "performances and optimizations", strictly speaking QHash::value() does not allocate iterators, since it relies on the internal nodes representation.

zzag added a comment.Jul 1 2018, 8:17 AM
In D13820#285503, @pino wrote:

Hardly an issue... Also, if you talk about "performances and optimizations", strictly speaking QHash::value() does not allocate iterators, since it relies on the internal nodes representation.

What do you mean by "allocate iterators"? IIRC, const_iterator is just a wrapper around internal node data structure. So, underneath, that's just a pointer.

pino added a comment.Jul 1 2018, 8:21 AM
In D13820#285517, @zzag wrote:
In D13820#285503, @pino wrote:

Hardly an issue... Also, if you talk about "performances and optimizations", strictly speaking QHash::value() does not allocate iterators, since it relies on the internal nodes representation.

What do you mean by "allocate iterators"? IIRC, const_iterator is just a wrapper around internal node data structure. So, underneath, that's just a pointer.

Sure, so an empty QVariant is just few mostly unallocated bytes., nothing to be worrying about.

In the end, your current patch is basically reimplementing QHash::value() for no good.

zzag updated this revision to Diff 36990.Jul 1 2018, 8:34 AM

Use QHash::value(const Key &key)

zzag added a comment.Jul 1 2018, 8:34 AM
In D13820#285522, @pino wrote:

In the end, your current patch is basically reimplementing QHash::value() for no good.

https://code.woboq.org/qt5/qtbase/src/corelib/tools/qhash.h.html#_ZNK5QHash5valueERKT_

Indeed.

zzag edited the summary of this revision. (Show Details)Jul 1 2018, 8:52 AM
zzag retitled this revision from Don't hash the role key twice in EffectWindowImpl::data to Use QHash::value() in EffectWindowImpl::data().Jul 1 2018, 8:54 AM
Closed by commit R108:6f173fc609c9: Use QHash::value() in EffectWindowImpl::data() (authored by zzag). · Explain WhyJul 2 2018, 12:08 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Meffects.cpp (4 lines)

Diff 37058

View Options

effects.cpp

Loading...