Use Key API in ResultSet
ClosedPublic

Authored by rnicole on Jul 23 2018, 9:18 AM.

Details

Summary

Depends on D14099

Notes:

  • Tests pass without many modifications outside of resultset.cpp/.h
  • mGenerator doesn't seem to be used?

Benchmarks

Run benchmarks:

DevelopD14099This patch
Current Rss usage [kb]: 40700Current Rss usage [kb]: 38564Current Rss usage [kb]: 39112
Peak Rss usage [kb]: 40700Peak Rss usage [kb]: 38564Peak Rss usage [kb]: 39112
Rss growth [kb]: 15920Rss growth [kb]: 13352Rss growth [kb]: 13432
Rss growth per entity [byte]: 3260Rss growth per entity [byte]: 2734Rss growth per entity [byte]: 2750
Rss without db [kb]: 29736Rss without db [kb]: 29248Rss without db [kb]: 30100
Percentage peak rss error: 0Percentage peak rss error: 0Percentage peak rss error: 0
On disk [kb]: 10788On disk [kb]: 9140On disk [kb]: 8836
Buffer size total [kb]: 898Buffer size total [kb]: 898Buffer size total [kb]: 898
Write amplification: 12.0075Write amplification: 10.1732Write amplification: 9.83485

Test Disk Usage:

DevelopD14099This patch
Free pages: 412Free pages: 309Free pages: 312
Total pages: 760Total pages: 599Total pages: 603
Used size: 1425408Used size: 1187840Used size: 1191936
Calculated key + value size: 856932Calculated key + value size: 702866Calculated key + value size: 702866
Calculated total db sizes: 970752Calculated total db sizes: 954368Calculated total db sizes: 933888
Main store on disk: 3112960Main store on disk: 2453504Main store on disk: 2469888
Total on disk: 3293184Total on disk: 2633728Total on disk: 2650112
Used size amplification: 1.66339Used size amplification: 1.68999Used size amplification: 1.69582
Write amplification: 3.63268Write amplification: 3.49071Write amplification: 3.51402

Diff Detail

Repository
R9 Sink
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rnicole requested review of this revision.Jul 23 2018, 9:18 AM
rnicole created this revision.
cmollekopf added inline comments.Jul 23 2018, 10:10 AM
common/queryrunner.cpp
274

This is not equivalent. You could clear() and then append, but for the sake of clarity I would actually prefer something along the lines of:

valuueCopy->aggregateIds() = [&] {
QVector<QByteArray> v;
v.reserve(result.aggregateIds.size());
std::transform(...)
return v;
}();

While this is slightly longer due to the temp variable, it makes clear that this is an assignment, and is probably similary efficient due to the implicit sharing of QVector, which makes assignment a very cheap operation (doesn't copy the data, but only the pointer).

rnicole updated this revision to Diff 38245.Jul 23 2018, 12:27 PM

Make assignation clear.

This should be temporary, until we convert ApplicationDomainType

rnicole marked an inline comment as done.Jul 23 2018, 12:27 PM
cmollekopf added inline comments.Jul 23 2018, 1:40 PM
common/queryrunner.cpp
274

Wrap it in a lambda please.

rnicole updated this revision to Diff 38255.Jul 23 2018, 2:09 PM
rnicole marked an inline comment as done.

Wrap assignment in lambda

cmollekopf accepted this revision.Jul 23 2018, 2:13 PM
This revision is now accepted and ready to land.Jul 23 2018, 2:13 PM
This revision was automatically updated to reflect the committed changes.