Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize inserts using reserve() and micro optimizations #3257

Closed
wants to merge 2 commits into from
Closed

Optimize inserts using reserve() and micro optimizations #3257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b7cf2e
Optimize inserts using reserve()
GermanAizek Nov 3, 2023
ccd6204
Added another microoptimizations
GermanAizek Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/core/Cutter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ QVector<QString> CutterCore::getCutterRCFilePaths() const
QVector<QString> result;
result.push_back(QFileInfo(QDir::home(), ".cutterrc").absoluteFilePath());
QStringList locations = QStandardPaths::standardLocations(QStandardPaths::AppConfigLocation);
result.reserve(locations.size() + 1);
for (auto &location : locations) {
result.push_back(QFileInfo(QDir(location), ".cutterrc").absoluteFilePath());
}
Expand Down Expand Up @@ -1851,6 +1852,7 @@ QVector<RegisterRefValueDescription> CutterCore::getRegisterRefValues()
}
RzListIter *it;
RzRegItem *ri;
result.reserve(ritems->length);
CutterRzListForeach (ritems, it, RzRegItem, ri) {
RegisterRefValueDescription desc;
desc.name = ri->name;
Expand Down Expand Up @@ -2806,8 +2808,10 @@ QList<BreakpointDescription> CutterCore::getBreakpoints()
CORE_LOCK();
QList<BreakpointDescription> ret;
// TODO: use higher level API, don't touch rizin bps_idx directly
for (int i = 0; i < core->dbg->bp->bps_idx_count; i++) {
if (auto bpi = core->dbg->bp->bps_idx[i]) {
const auto* breakpoints = core->dbg->bp;
ret.reserve(breakpoints->bps_idx_count);
for (int i = 0; i < breakpoints->bps_idx_count; i++) {
if (auto bpi = breakpoints->bps_idx[i]) {
ret.push_back(breakpointDescriptionFromRizin(i, bpi));
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/core/MainWindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,8 @@ void MainWindow::initDocks()

auto makeActionList = [this](QList<CutterDockWidget *> docks) {
QList<QAction *> result;
for (auto dock : docks) {
result.reserve(docks.size());
for (const auto* dock : docks) {
if (dock != nullptr) {
result.push_back(dock->toggleViewAction());
} else {
Expand Down
5 changes: 3 additions & 2 deletions src/widgets/ColorPicker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ void ColorPickArea::paintEvent(QPaintEvent *event)
{
QPainter p(this);

for (int x = event->rect().x(); x <= event->rect().right(); x++) {
for (int y = event->rect().y(); y <= event->rect().bottom(); y++) {
const auto& erect = event->rect();
for (int x = erect.x(); x <= erect.right(); x++) {
for (int y = erect.y(); y <= erect.bottom(); y++) {
qhelpers::ColorFloat h, s, v;
QColor c = pointToColor(x, y);
c.getHsvF(&h, &s, &v);
Expand Down
2 changes: 1 addition & 1 deletion src/widgets/ColorThemeListView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ void ColorSettingsModel::updateTheme()
theme.clear();
ColorThemeWorker::Theme obj = ThemeWorker().getTheme(Config()->getColorTheme());

for (auto it = obj.constBegin(); it != obj.constEnd(); it++) {
for (auto it = obj.constBegin(); it != obj.constEnd(); ++it) {
theme.push_back({ it.key(), it.value(), false });
}

Expand Down
5 changes: 3 additions & 2 deletions src/widgets/DecompilerWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,10 @@ DecompilerWidget::DecompilerWidget(MainWindow *main)
selectedDecompilerId = "ghidra";
}
for (Decompiler *dec : decompilers) {
ui->decompilerComboBox->addItem(dec->getName(), dec->getId());
auto* combobox = ui->decompilerComboBox;
combobox->addItem(dec->getName(), dec->getId());
if (dec->getId() == selectedDecompilerId) {
ui->decompilerComboBox->setCurrentIndex(ui->decompilerComboBox->count() - 1);
combobox->setCurrentIndex(combobox->count() - 1);
}
}
decompilerSelectionEnabled = decompilers.size() > 1;
Expand Down
3 changes: 2 additions & 1 deletion src/widgets/DisassemblerGraphView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,9 @@ void DisassemblerGraphView::loadCurrentGraph()
rz_core_print_disasm(core, bbi->addr, buf.get(), (int)bbi->size, (int)bbi->size, NULL,
&options);

auto vecVisitor = CutterPVector<RzAnalysisDisasmText>(vec.get());
auto vecVisitor = CutterPVector<RzAnalysisDisasmText>(options.vec);
auto iter = vecVisitor.begin();
db.instrs.reserve(rz_pvector_len(options.vec));
while (iter != vecVisitor.end()) {
RzAnalysisDisasmText *op = *iter;
Instr instr;
Expand Down
2 changes: 1 addition & 1 deletion src/widgets/DisassemblyWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ void DisassemblyWidget::highlightCurrentLine()

// Highlight the current line
QTextEdit::ExtraSelection highlightSelection;
highlightSelection.cursor = cursor;
highlightSelection.cursor = std::move(cursor);
highlightSelection.cursor.movePosition(QTextCursor::Start);
while (true) {
RVA lineOffset = DisassemblyPreview::readDisassemblyOffset(highlightSelection.cursor);
Expand Down
18 changes: 9 additions & 9 deletions src/widgets/GraphGridLayout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,12 +806,12 @@ void calculateSegmentOffsets(std::vector<EdgeSegment> &segments, std::vector<int
}
maxSegment.setRange(0, H, -leftColumWidth);
while (rightSideIt != nodeRightSide.end() && rightSideIt->x + 1 < x) {
rightSideIt++;
++rightSideIt;
}
while (rightSideIt != nodeRightSide.end() && rightSideIt->x + 1 == x) {
maxSegment.setRange(rightSideIt->y0, rightSideIt->y1 + 1,
rightSideIt->size - leftColumWidth);
rightSideIt++;
++rightSideIt;
}

while (nextSegmentIt != segments.end() && nextSegmentIt->x == x
Expand All @@ -823,7 +823,7 @@ void calculateSegmentOffsets(std::vector<EdgeSegment> &segments, std::vector<int
y += nextSegmentIt->spacingOverride ? nextSegmentIt->spacingOverride : segmentSpacing;
maxSegment.setRange(nextSegmentIt->y0, nextSegmentIt->y1 + 1, y);
edgeOffsets[nextSegmentIt->edgeIndex] = y;
nextSegmentIt++;
++nextSegmentIt;
}

auto firstRightSideSegment = nextSegmentIt;
Expand All @@ -836,19 +836,19 @@ void calculateSegmentOffsets(std::vector<EdgeSegment> &segments, std::vector<int

maxSegment.setRange(0, H, -rightColumnWidth);
while (leftSideIt != nodeLeftSide.end() && leftSideIt->x < x) {
leftSideIt++;
++leftSideIt;
}
while (leftSideIt != nodeLeftSide.end() && leftSideIt->x == x) {
maxSegment.setRange(leftSideIt->y0, leftSideIt->y1 + 1,
leftSideIt->size - rightColumnWidth);
leftSideIt++;
++leftSideIt;
}
while (nextSegmentIt != segments.end() && nextSegmentIt->x == x) {
int y = maxSegment.rangeMaximum(nextSegmentIt->y0, nextSegmentIt->y1 + 1);
y += nextSegmentIt->spacingOverride ? nextSegmentIt->spacingOverride : segmentSpacing;
maxSegment.setRange(nextSegmentIt->y0, nextSegmentIt->y1 + 1, y);
edgeOffsets[nextSegmentIt->edgeIndex] = y;
nextSegmentIt++;
++nextSegmentIt;
}
auto rightSideMiddle = std::max(maxSegment.rangeMaximum(0, H), 0);
rightSideMiddle =
Expand Down Expand Up @@ -916,10 +916,10 @@ static void centerEdges(std::vector<int> &segmentOffsets, const std::vector<int>
int offset = segmentOffsets[it->index];
left = std::min(left, offset);
right = std::max(right, offset);
it++;
++it;
}
int spacing = (edgeColumnWidth[chunkStart->x] - (right - left)) / 2 - left;
for (auto segment = chunkStart; segment != it; segment++) {
for (auto segment = chunkStart; segment != it; ++segment) {
if (segment->start) {
segmentOffsets[segment->index] += spacing;
}
Expand Down Expand Up @@ -1556,7 +1556,7 @@ static void createInequalitiesFromSegments(std::vector<Segment> segments,
++it;
}
if (startPos->first < segment.y0) {
startPos++;
++startPos;
}
lastSegments.erase(startPos, it); // erase segments covered by current one
lastSegments[segment.y0] = {
Expand Down
8 changes: 6 additions & 2 deletions src/widgets/HeapBinsGraphView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,9 @@ void HeapBinsGraphView::display_single_linked_list(QVector<GraphHeapChunk> chunk
addBlock(gbBin, content);

// add the graph blocks for the chunks
GraphLayout::GraphBlock gbChunk;
gbChunk.edges.reserve(chunks.size());
for (int i = 0; i < chunks.size(); i++) {
GraphLayout::GraphBlock gbChunk;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you have profiling results proving that this is performance critical to reuse this block, I recommend keeping the declaration where it was. Reusing it without clearly reinitializing everything has risk of getting some old values from previous iterations, if not now, then as rest of code changes. std::move at the end isn't reliable way for ensuring the state of reused block afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karliss, will this code fix this situation?

    // add the blocks for the chunks
    GraphLayout::GraphBlock gbChunk;
    gbChunk.edges.reserve(chunks.size() * 2);
    for (int i = 0; i < chunks.size(); i++) {
        gbChunk.entry = chunks[i].addr;
        gbChunk.edges.emplace_back(chunks[i].fd);
        gbChunk.edges.emplace_back(chunks[i].bk);

        // if last chunk and there is message then show it in the chunk
        if (i == chunks.size() - 1 && heapBin->message) {
            chunks[i].content += "\n" + QString(heapBin->message);
        }

        addBlock(gbChunk, chunks[i].content, chunks[i].addr);
        memset(&gbChunk, 0, sizeof(gbChunk));
    }

gbChunk.entry = chunks[i].addr;

if (tcache && chunks[i].fd) {
Expand All @@ -101,6 +102,7 @@ void HeapBinsGraphView::display_single_linked_list(QVector<GraphHeapChunk> chunk
}

addBlock(gbChunk, chunks[i].content, chunks[i].addr);
memset(&gbChunk, 0, sizeof(gbChunk));
}

// add the END block if no message
Expand All @@ -126,8 +128,9 @@ void HeapBinsGraphView::display_double_linked_list(QVector<GraphHeapChunk> chunk
addBlock(gbBin, content, heapBin->addr);

// add the blocks for the chunks
GraphLayout::GraphBlock gbChunk;
gbChunk.edges.reserve(chunks.size() * 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this is a pesimization not an optimization. No block will have that many edges.

And since you are both reusing the block and later moving it away, the reserving here is at best useless, and in the worst case if move doesn't succeed you will have overall reserved O(n^2) memory for a list.

Knowing that there are always two edges might as well do something like

gbChunk.edges = { 
    GraphEdge(chunks[i].fd), 
    GraphEdge(chunks[i].bk) 
};

for (int i = 0; i < chunks.size(); i++) {
GraphLayout::GraphBlock gbChunk;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same ideas as previous comment about block reuse.

Copy link
Contributor Author

@GermanAizek GermanAizek Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question as above.

gbChunk.entry = chunks[i].addr;
gbChunk.edges.emplace_back(chunks[i].fd);
gbChunk.edges.emplace_back(chunks[i].bk);
Expand All @@ -138,6 +141,7 @@ void HeapBinsGraphView::display_double_linked_list(QVector<GraphHeapChunk> chunk
}

addBlock(gbChunk, chunks[i].content, chunks[i].addr);
memset(&gbChunk, 0, sizeof(gbChunk));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/widgets/HexdumpWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ void HexdumpWidget::updateParseWindow(RVA start_address, int size)
tempConfig.set("asm.arch", arch).set("asm.bits", bits).set("cfg.bigendian", bigEndian);

ui->hexDisasTextEdit->setPlainText(
selectedCommand != "" ? Core()->cmdRawAt(
!selectedCommand.isEmpty() ? Core()->cmdRawAt(
QString("%1 @! %2").arg(selectedCommand).arg(size), start_address)
: "");
} else {
Expand Down
1 change: 1 addition & 0 deletions src/widgets/RegisterRefsWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ void RegisterRefsWidget::refreshRegisterRef()
registerRefModel->beginResetModel();

registerRefs.clear();
registerRefs.reserve(Core()->getRegisterRefs().size());
for (const RegisterRef &reg : Core()->getRegisterRefs()) {
RegisterRefDescription desc;

Expand Down
1 change: 1 addition & 0 deletions src/widgets/RizinGraphWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ void GenericRizinGraphView::loadCurrentGraph()
auto edges = block["out_nodes"];
GraphLayout::GraphBlock layoutBlock;
layoutBlock.entry = id;
layoutBlock.edges.reserve(edges.size());
for (auto edge : edges) {
auto targetId = edge.toUt64();
layoutBlock.edges.emplace_back(targetId);
Expand Down
1 change: 1 addition & 0 deletions src/widgets/StackWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ void StackModel::reload()

beginResetModel();
values.clear();
values.reserve(stackItems.size());
for (const AddrRefs &stackItem : stackItems) {
Item item;

Expand Down
Loading