ProgramMemory: only copy `mValues` on write by firewave · Pull Request #6646 · cppcheck-opensource/cppcheck

firewave · 2024-07-29T14:26:28Z

No description provided.

firewave · 2024-07-29T14:27:28Z

Some unit tests (e.g.TestBufferOverrun) still fail. The test I added for ProgramMemory works. Maybe I need to add test for each of the functions (well, I should).

There may also be unnecessary copies introduced by my changes related to getProgramState().

pfultz2 · 2024-07-29T15:22:43Z

+    copyOnWrite();
+    pm.copyOnWrite();
+
+    mValues->swap(*pm.mValues);


Should be mValues.swap(pm.mValues).

pfultz2 · 2024-07-29T15:32:29Z

+
+void ProgramMemory::copyOnWrite()
+{
+    if (!mCopyValues)


There is no need for the mCopyValues variable. Just check mValues.use_count() == 1 instead.

pfultz2 · 2024-07-29T15:35:03Z

+        mValues = other.mValues;
+        mCopyValues = true;
+        return *this;
+    }


Just use the default-generated copy constructor and assignment.

Without the need for mCopyValues I can do taht now.

It would have also required the move constructor/assignment.

pfultz2 · 2024-07-29T15:37:13Z

    using ProgramState = ProgramMemory::Map;

-    virtual ProgramState getProgramState() const = 0;
+    virtual std::shared_ptr<ProgramState> getProgramState() const = 0;


Why is this changed to a shared_ptr? Its never shared.

Because it is used to construct a ProgramMemory object in ValueFlowAnalyzer::evaluateInt().

But you can just move it instead.

firewave · 2024-07-29T16:31:53Z

With the first round of review comments addressed the unit tests now all pass.

Here some preliminary numbers (without having looked at the profiling data if everything is on the up and up).

-D__GNUC__ --check-level=exhaustive ../lib/utils.cpp

Clang 17 883,929,117 -> 878,381,287

The example from https://trac.cppcheck.net/ticket/10765#comment:4:

Clang 17 8,156,089,970 -> 4,782,261,960

pfultz2 · 2024-07-29T16:57:41Z

+    ProgramMemory() : mValues(new Map()) {}

-    explicit ProgramMemory(Map values) : mValues(std::move(values)) {}
+    explicit ProgramMemory(std::shared_ptr<Map> values) : mValues(std::move(values)) {}


This should not need to be changed to a shared_ptr, you can implement it as:

explicit ProgramMemory(Map values) : mValues(new Map()) { *mValues = std::move(values); }

Done. Did it even simpler by moving it into the constructor.

firewave · 2024-07-29T17:02:31Z


    Map::iterator begin() {
-        return mValues.begin();
+        return mValues->begin();


lacks copyOnWrite() as it returns something mutable.

firewave · 2024-07-29T17:02:42Z


    Map::iterator end() {
-        return mValues.end();
+        return mValues->end();


lacks copyOnWrite() as it returns something mutable.

firewave · 2024-07-29T18:08:58Z

I added some more early outs (might split those into a separate commit later) and removed some unused code.

firewave · 2024-07-29T20:15:50Z

Unfortunately this does not seem to help much with the selfcheck. ☹

Co-authored-by: Paul Fultz II <paul.fultz@amd.com>

…ang-tidy warning

chrchr-github · 2024-07-30T09:41:20Z

+    // TODO: how to delay until we actuallly modify?
+    copyOnWrite();
+
+    for (auto it = mValues->begin(); it != mValues->end();) {


Can't this be written as remove-erase?

Possibly but that would not fix the TODO and just clean up the code.

If the call to remove_if() returns end(), no modification takes place, or am I missing something?

But we would have to determine if a modification happens, then make the copy and afterwards iterate it again because the existing result is based on a different container.

We only need to make this optimization if it is a hot spot.

remove_if wouldn't work here anyway: https://stackoverflow.com/questions/9210014/remove-elements-from-an-unordered-map-fulfilling-a-predicate

chrchr-github · 2024-07-30T09:42:12Z

+    ProgramMemory() : mValues(new Map()) {}

-    explicit ProgramMemory(Map values) : mValues(std::move(values)) {}
+    explicit ProgramMemory(Map values) : mValues(new Map(std::move(values))) {}


make_shared?

That would construct a shared_ptr object with an shared_ptr which is unnecessary.

firewave force-pushed the pm-shared branch from 8596e05 to 4f43a50 Compare July 29, 2024 15:00

pfultz2 reviewed Jul 29, 2024

View reviewed changes

firewave changed the title ~~ProgramMemory: copy-on-write mValues [skip ci]~~ ProgramMemory: copy-on-write mValues Jul 29, 2024

firewave force-pushed the pm-shared branch from 4f43a50 to 0fc028e Compare July 29, 2024 16:19

pfultz2 reviewed Jul 29, 2024

View reviewed changes

firewave commented Jul 29, 2024

View reviewed changes

firewave force-pushed the pm-shared branch 3 times, most recently from 6704722 to 25ec10a Compare July 29, 2024 18:08

firewave force-pushed the pm-shared branch 2 times, most recently from 30284cc to 4454389 Compare July 29, 2024 19:27

firewave changed the title ~~ProgramMemory: copy-on-write mValues~~ ProgramMemory: only copy mValues on write Jul 29, 2024

firewave force-pushed the pm-shared branch from 4454389 to 051fab6 Compare July 29, 2024 19:35

firewave force-pushed the pm-shared branch from 051fab6 to 0a0e873 Compare July 29, 2024 20:27

firewave and others added 6 commits July 30, 2024 08:42

ProgramMemory: only copy mValues on write

28680db

Co-authored-by: Paul Fultz II <paul.fultz@amd.com>

ProgramMemory: added some early outs

a0829f5

ProgramMemory: removed unused insert() functions

235691e

testprogrammemory.cpp: fixed functionConst selfcheck warning

ce6d87f

programmemory.cpp: fixed modernize-make-shared clang-tidy warning

3af3159

programmemory.cpp: mitigated performance-unnecessary-value-param cl…

809b0e6

…ang-tidy warning

firewave force-pushed the pm-shared branch from 0a0e873 to 809b0e6 Compare July 30, 2024 06:42

firewave marked this pull request as ready for review July 30, 2024 06:42

chrchr-github reviewed Jul 30, 2024

View reviewed changes

chrchr-github merged commit 53e55be into cppcheck-opensource:main Jul 31, 2024

firewave deleted the pm-shared branch July 31, 2024 11:56

Conversation

firewave commented Jul 29, 2024

Uh oh!

firewave commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave commented Jul 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave commented Jul 29, 2024

Uh oh!

firewave commented Jul 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

firewave commented Jul 29, 2024 •

edited

Loading