ProgramMemory: only copy `mValues` on write #6646

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

lib/programmemory.cpp

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


Should be mValues.swap(pm.mValues).

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

lib/programmemory.cpp

+
+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

lib/programmemory.h

+        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

lib/valueflow.cpp

@@ -896,7 +896,7 @@ struct ValueFlowAnalyzer : Analyzer {

    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

lib/programmemory.h


-    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

lib/programmemory.h

@@ -134,19 +134,19 @@ struct ProgramMemory {
    void insert(const ProgramMemory &pm);

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


lacks copyOnWrite() as it returns something mutable.

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

lib/programmemory.h

    }

    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 <[email protected]>

…ang-tidy warning

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

lib/programmemory.cpp

+    // 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

lib/programmemory.h


-    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 <[email protected]>

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 danmar:main Jul 31, 2024
63 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ProgramMemory: only copy `mValues` on write #6646

ProgramMemory: only copy `mValues` on write #6646

firewave commented Jul 29, 2024

firewave commented Jul 29, 2024 •

edited

Loading

pfultz2 Jul 29, 2024

firewave Jul 29, 2024

pfultz2 Jul 29, 2024

firewave Jul 29, 2024

firewave Jul 29, 2024

pfultz2 Jul 29, 2024

firewave Jul 29, 2024

firewave Jul 29, 2024

pfultz2 Jul 29, 2024

firewave Jul 29, 2024

pfultz2 Jul 29, 2024

firewave Jul 29, 2024

firewave commented Jul 29, 2024

pfultz2 Jul 29, 2024

firewave Jul 29, 2024

firewave Jul 29, 2024

firewave Jul 29, 2024

firewave Jul 29, 2024

firewave Jul 29, 2024

firewave commented Jul 29, 2024

firewave commented Jul 29, 2024

chrchr-github Jul 30, 2024

firewave Jul 30, 2024

chrchr-github Jul 30, 2024

firewave Jul 30, 2024

chrchr-github Jul 31, 2024

chrchr-github Jul 30, 2024

firewave Jul 30, 2024


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

ProgramMemory: only copy mValues on write #6646

ProgramMemory: only copy mValues on write #6646

Conversation

firewave commented Jul 29, 2024

firewave commented Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firewave commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firewave commented Jul 29, 2024

firewave commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProgramMemory: only copy `mValues` on write #6646

ProgramMemory: only copy `mValues` on write #6646

firewave commented Jul 29, 2024 •

edited

Loading