ENH: Fixed Coverity 2023.6.2 warnings in itkImageRegion.h #4334

issakomi · 2023-11-18T14:59:43Z

Coverity 2023.6.2:

275       bool
276       IsInside(const Self & otherRegion) const
277       {
>>>     CID 331225:  Performance inefficiencies  (AUTO_CAUSES_COPY)
>>>     Using the "auto" keyword without an "&" causes the copy of an object of type "itk::ImageRegion<3u>::IndexType".
278         const auto otherIndex = otherRegion.m_Index;

275       bool
276       IsInside(const Self & otherRegion) const
277       {
278         const auto otherIndex = otherRegion.m_Index;
>>>     CID 331206:  Performance inefficiencies  (AUTO_CAUSES_COPY)
>>>     Using the "auto" keyword without an "&" causes the copy of an object of type "itk::ImageRegion<3u>::SizeType".
279         const auto otherSize = otherRegion.m_Size;

N-Dekker · 2023-11-19T20:23:42Z

Thanks @issakomi. But although it's nice if such a warning can be avoided, it isn't really obvious to me that your PR would really improve the performance of IsInside(const Self &). Did you observe any performance improvement?

N-Dekker · 2023-11-19T21:22:03Z

Modules/Core/Common/include/itkImageRegion.h

-    const auto otherIndex = otherRegion.m_Index;
-    const auto otherSize = otherRegion.m_Size;
+    const auto & otherIndex = otherRegion.m_Index;
+    const auto & otherSize = otherRegion.m_Size;


Both Index and Size are trivially copyable (having a super-fast compiler-generated copy-constructor and an empty compiler-generated destructor). When declaring otherIndex and otherSize as reference, each access to the data referred to by those variables (as in the for-loop below) will have an extra indirection. Both otherIndex[i] and otherSize[i] are accessed multiple times. That's why I wonder: is this really a performance improvement?

I understood this as a change to silence a warning.

IMHO, otherRegion object is already passed by constant reference to the function. Not sure that there are some "extra indirection" costs for otherIndex and otherSize, IMHO they are just aliases, but maybe you are right. Just close the PR if you think there are some costs for "extra indirection" of constant reference.

Coverity 2023.6.2: 275 bool 276 IsInside(const Self & otherRegion) const 277 { >>> CID 331225: Performance inefficiencies (AUTO_CAUSES_COPY) >>> Using the "auto" keyword without an "&" causes the copy of an object of type "itk::ImageRegion<3u>::IndexType". 278 const auto otherIndex = otherRegion.m_Index; 275 bool 276 IsInside(const Self & otherRegion) const 277 { 278 const auto otherIndex = otherRegion.m_Index; >>> CID 331206: Performance inefficiencies (AUTO_CAUSES_COPY) >>> Using the "auto" keyword without an "&" causes the copy of an object of type "itk::ImageRegion<3u>::SizeType". 279 const auto otherSize = otherRegion.m_Size;

N-Dekker · 2023-11-20T13:03:07Z

Thanks for adjusting the commit message, @issakomi Of course it would have been nice if the commit would yield a significant performance improvement, but I still doubt if it would. It does obviously fix the CID 331225 (AUTO_CAUSES_COPY) warning, although I feel that the warning is undeserved for types that are trivially copyable.

Another way to address CID 331225 (AUTO_CAUSES_COPY) might be to just not use auto, in this case. I mean:

const IndexType otherIndex = otherRegion.m_Index;
const SizeType otherSize = otherRegion.m_Size;

Would you also like that?

Anyway, I'm kinda neutral to the PR now. But generally speaking, I think it's OK to copy a trivially copyable object, instead of using a const-reference to the object. Copying (rather than referencing) will avoid an extra indirection when accessing the object. Moreover, it will ease making code thread-safe. (Consider two threads referring to one and the same object versus each thread having its own copy of the object.)

dzenanz · 2023-11-20T13:45:33Z

If you re-open this PR, I could merge it. Assuming you don't create a new PR, which removes the auto specifier (per Niels' suggestion).

issakomi · 2023-11-21T19:18:04Z

I have done some tests. Even if this PR is not important for me at all, it is just a warning from Coverity and and I don't heavily use this IsInside function in my app. I will not re-open the PR.

I feel that the warning is undeserved for types that are trivially copyable.

It depends on dimension of the itk::ImageRegion. The larger the dimension, the greater the difference, of course copy takes more time.

I have done some tests with dimensions 3 and 10. With auto & it seems to be always faster, specially with dimension 10. With dimension 3 there was never a slowdown, BTW, also always a little faster.
Honestly, I have to admit that I can not comment on "extra indirection" argument. I have read somewhere, that it doesn't make much sense to pass built-in types as const-reference, e.g.
void f(int x) and void f(const int & x)
and I don't have more information about costs of "indirection" of references. I can image too that in some situation having some object inside a scope could give more room for optimization, not sure. But generally I imagine a reference as an alias, just another name for something.

Moreover, it will ease making code thread-safe.

I am afraid that thread-safety argument is not applicable in particular case, otherRegion is passed by const-reference to the function.

Here is the test. But it is a little tricky, the times vary, the test have to be run many times to get an idea (and it would be better to prepare different binaries before testing).

10000 IsInside calls:

With dim 10 and auto &
48283 ns
48543 ns
53882 ns
53138 ns
45444 ns

With dim 10 and auto
73654 ns
74460 ns
73431 ns
74650 ns
75135 ns

With dim 3 and auto &
32371 ns
32309 ns
32283 ns
32344 ns
32481 ns
32546 ns

With dim 3 and auto
36645 ns
36453 ns
34738 ns
36289 ns
34679 ns

#include <itkImageRegion.h>
#include <iostream>
#include <vector>
#include <chrono>
#include <random>

int main(int, char **)
{
  ///////////////////////////////////////////////////////////////////////////////
  //
  //
  //
  //
  constexpr unsigned int dim = 10;
  constexpr unsigned int num_tests = 10000;
  //
  //
  //
  //
  ///////////////////////////////////////////////////////////////////////////////

  using RegionType = itk::ImageRegion<dim>;
  using IndexType = RegionType::IndexType;
  using SizeType = RegionType::SizeType;

  RegionType region;
  {
    SizeType size;
    for (unsigned int x = 0; x < dim; ++x)
    {
      size[x] = 2048;
    }
    IndexType index{};
    region.SetSize(size);
    region.SetIndex(index);
  }

  std::vector<RegionType> regions;
  for (unsigned int x{}; x < num_tests; ++x)
  {
    RegionType r;
    SizeType s;
    IndexType i{};
    for (unsigned int k{}; k < dim; ++k)
    {
      s[k] = x + 1;
    }
    r.SetSize(s);
    r.SetIndex(i);
    regions.push_back(r);
  }

  std::vector<bool> results;

  ///////////////////////////////////////////////////////////////////////////////
  //
  //
  //
  //
  const auto t0 = std::chrono::steady_clock::now();
  for (size_t x{}; x < regions.size(); ++x)
  {
    const bool k = region.IsInside(regions.at(x));
    results.push_back(k);
  }
  const auto t1 = std::chrono::steady_clock::now();
  const auto ts = std::chrono::duration_cast<std::chrono::nanoseconds>(t1 - t0);
  //
  //
  //
  //
  ///////////////////////////////////////////////////////////////////////////////


  // To avoid "optimized away" scenario
  {
    const unsigned long long seed =
      std::chrono::high_resolution_clock::now().time_since_epoch().count();
    std::mt19937_64 mtrand(seed);
    const unsigned int x = mtrand() % num_tests;
    std::cout << x << " " << static_cast<int>(results.at(x)) << std::endl;
  }

  std::cout << "Time: " << ts.count() << " ns" <<std::endl;
  return 0;
}

Edit:
BTW, LLVM had similar AUTO_CAUSES_COPY warnings (Coverity calls "low impact defects") and addressed them
https://reviews.llvm.org/D149074
Of course Coverity can make many false positives. BTW, I am usually a little skeptic about silencing of warnings from all the analysis tools, it can produce more bugs easily.

N-Dekker

Thank you for further investigating this topic, @issakomi I think you convinced me that your proposed change is a proper way to address the Coverity warnings, at least in this particular case. In general I think we should still allow code of the form const auto x = expression in our code base (for the reasons I mentioned before), so I hope this is a special (exceptional) case. Anyway, approved 👍

N-Dekker · 2023-11-22T13:27:22Z

By the way, I think I will still ask around what other C++ programmers think about those AUTO_CAUSES_COPY warnings. Specifically, how do they feel about Coverity producing those warnings, even for trivially copyable objects? I will let you know if I get any new insight on this topic!

For the record, Coverity 2023.6.0 Release Notes claims that:

The AUTO_CAUSES_COPY checker no longer reports when small objects are copied.

@issakomi Can you possibly figure out for us what Coverity means by "small objects", in this context? Apparently AUTO_CAUSES_COPY is reported for itk::Size<3>, but is it also reported for itk::Size<2>?

N-Dekker · 2023-12-12T09:42:30Z

@issakomi Can you possibly still figure out whether the AUTO_CAUSES_COPY warning appears on both itk::Size<3> and itk::Size<2>, or just on itk::Size<3>?

The Coverity warning supposedly does not appear for "small objects", so I'm just curious now where they draw the line between small and large!

issakomi · 2023-12-18T06:09:11Z

The warning was about 3D region only, I am not sure about the limit for "small objects", maybe 8 or 10 bytes or something, I could not find information about the limit, only these lines in "2023.6.0 Release Notes"

SAT-17064
    The AUTO_CAUSES_COPY checker now reports on functions and methods declared as auto, as well as on assignments to auto variables.

SAT-44114
    The AUTO_CAUSES_COPY checker no longer reports when small objects are copied.

SAT-44127
    The AUTO_CAUSES_COPY checker now suggests the use of const auto& and only reports when that suggestion is applicable.

N-Dekker · 2023-12-18T13:23:52Z

Thanks @issakomi Interesting! But then, maybe they might also consider the counter-part:

// In a 2D world:
const Size<2> & otherSize = otherRegion.m_Size;

Might then produce a warning, saying "Using & -- for small objects, copying is preferred"! 😺

N-Dekker · 2023-12-18T13:46:04Z

@issakomi Can you possibly still check whether or not the AUTO_CAUSES_COPY warning appears when an object is small (at most 8 bytes) but "non-trivial"? Like the following String class (also at https://godbolt.org/z/fGGrvGcqE):

class String
{
public:
    String(const String& arg)
        :
        m_ptr(new char[strlen(arg.m_ptr.get()) + 1])
    {
        strcpy(m_ptr.get(), arg.m_ptr.get());
    }

    std::unique_ptr<char[]> m_ptr;
};

issakomi · 2023-12-18T14:22:39Z

I have put following code in my app:

{
	using RegionType2 = itk::ImageRegion<2>;
	RegionType2 region2{};
	const auto otherSize2 = region2.GetSize();
	std::cout << otherSize2[0] << " " << otherSize2[1] << std::endl;
}
{
	using RegionType3 = itk::ImageRegion<3>;
	RegionType3 region3{};
	const auto otherSize3 = region3.GetSize();
	std::cout << otherSize3[0] << " " << otherSize3[1] << " " << otherSize3[2] << std::endl;
}

This triggered one defect:

New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)


** CID 337866:  Performance inefficiencies  (AUTO_CAUSES_COPY)
/home/r/alizams/AlizaMS/main.cpp: 510 in main()


*** CID 337866:  Performance inefficiencies  (AUTO_CAUSES_COPY)
/home/r/alizams/AlizaMS/main.cpp: 510 in main()
504             const auto otherSize2 = region2.GetSize();
505             std::cout << otherSize2[0] << " " << otherSize2[1] << std::endl;
506     }
507     {
508             using RegionType3 = itk::ImageRegion<3>;
509             RegionType3 region3{};
>>>     CID 337866:  Performance inefficiencies  (AUTO_CAUSES_COPY)
>>>     Using the "auto" keyword without an "&" causes the copy of an object of type "itk::ImageRegion<3u>::SizeType".
510             const auto otherSize3 = region3.GetSize();
511             std::cout << otherSize3[0] << " " << otherSize3[1] << " " << otherSize3[2] << std::endl;
512     }

itk::ImageRegion<2> didn't trigger the defect, only itk::ImageRegion<3>

There is also documentation for every "checker", but nasty additional registration at "Synopsys community" seems to be required (or I didn't find how else to see it).

I can check your String code later, the number of scans per days is limited.

issakomi · 2023-12-19T16:45:29Z

@N-Dekker I can run a scan today, it is OK to add the code like in main? BTW, I have added default ctor, without it I don't know how to use the class.

test.cpp: In function ‘int main()’:
test.cpp:26:16: error: no matching function for call to ‘String::String()’
   26 |         String s;
      |                ^
test.cpp:9:9: note: candidate: ‘String::String(const String&)’
    9 |         String(const String& arg)
      |         ^~~~~~
test.cpp:9:9: note:   candidate expects 1 argument, 0 provided

#include <iostream>
#include <memory>
#include <cstring>

class String
{
public:
	String() {}
	String(const String& arg)
		:
		m_ptr(new char[strlen(arg.m_ptr.get()) + 1])
	{
		strcpy(m_ptr.get(), arg.m_ptr.get());
	}
	std::unique_ptr<char[]> m_ptr;
};

void Check(const String& arg)
{
	const auto copyOfString = arg;
	std::cout << copyOfString.m_ptr.get() << std::endl;
}

int main()
{
	String s;
	s.m_ptr = std::unique_ptr<char[]>(new char[4]{'a','b','c','\0'});
	Check(s);
	return 0;
}

issakomi · 2023-12-19T17:11:46Z

Done.
There are no new defects with const auto copyOfString = arg;

Might then produce a warning, saying "Using & -- for small objects, copying is preferred"! 😺

Also const auto & copyOfString = arg; doesn't trigger a defect.

N-Dekker · 2023-12-19T17:17:50Z

There are no new defects with const auto copyOfString = arg;

Thanks @issakomi So you mean that there is no warning on const auto copyOfString = arg, right?

issakomi · 2023-12-19T17:20:12Z

There are no new defects with const auto copyOfString = arg;

Thanks @issakomi So you mean that there is no warning on const auto copyOfString = arg, right?

Yes. I guess arg is small enough.

Edit:
BTW, String doesn't want to be trivial without user-defined copy-ctor too and deletes ctors.

#include <iostream>
#include <memory>
#include <type_traits>

class String
{
public:
	std::unique_ptr<char[]> m_ptr;
};

class StringSimple
{
public:
	char * m_ptr;
};

int main()
{
	std::cout << std::is_trivial<String>() << '\n'
		<< std::is_trivial<StringSimple>() << std::endl;
	return 0;
}

r@deb2:~/Desktop$ ./a.out 
0
1

N-Dekker · 2023-12-19T17:46:02Z

Yes. I guess arg is small enough.

Thanks again. In this case, I think const auto copyOfString = arg would deserve a "Performance inefficiencies (AUTO_CAUSES_COPY)" warning, because it calls an expensive (non-trivial) copy-constructor. It would have been nice if Coverity would produce the warning whenever the type of the copied object is not std::is_trivially_copy_constructible.

Regarding your Edit:

BTW, String doesn't want to be trivial without user-defined copy-ctor too and deletes ctors.

OK, but then it would no longer be copyable, so AUTO_CAUSES_COPY would not apply anyway.

issakomi · 2023-12-19T17:54:32Z

It would have been nice if Coverity would produce the warning whenever the type of the copied object is not std::is_trivially_copy_constructible.

Yes. Next time I'll try to use a huge new char[100000] array or something, I wonder if the heavy strcpy will be detected or not. Perhaps my char[4] was too small. (Probably not, this could happen at runtime as well).
But honestly I'm not a Coverity expert, maybe you can join the Synopsys community and talk to them.

issakomi · 2023-12-19T18:25:55Z

I have done this crazy test with new char[100000]. Coverity didn't detect the inefficiency of the copy ctor and const auto copyOfString = arg;
:)

	String s;
	constexpr size_t asize = 100000;
	s.m_ptr = std::unique_ptr<char[]>(new char[asize]{
	'a','b','c','a','b','c','a','b','c','a',
<9998 lines cut>
	'a','b','c','a','b','c','a','b','c','\0'});
	Check(s);

    Analysis Summary:
       New defects found: 0

github-actions bot added type:Enhancement Improvement of existing methods or implementation area:Core Issues affecting the Core module labels Nov 18, 2023

dzenanz approved these changes Nov 18, 2023

View reviewed changes

N-Dekker reviewed Nov 19, 2023

View reviewed changes

issakomi force-pushed the auto1 branch from b359f3d to 28a8f91 Compare November 20, 2023 04:22

issakomi changed the title ~~ENH: Performance improvement in itkImageRegion.h~~ ENH: Fixed Coverity 2023.6.2 warnings in itkImageRegion.h Nov 20, 2023

issakomi closed this Nov 20, 2023

dzenanz reopened this Nov 21, 2023

N-Dekker approved these changes Nov 22, 2023

View reviewed changes

N-Dekker merged commit 9d2364f into InsightSoftwareConsortium:master Nov 22, 2023
10 checks passed

issakomi deleted the auto1 branch November 22, 2023 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Fixed Coverity 2023.6.2 warnings in itkImageRegion.h #4334

ENH: Fixed Coverity 2023.6.2 warnings in itkImageRegion.h #4334

issakomi commented Nov 18, 2023 •

edited

Loading

N-Dekker commented Nov 19, 2023

N-Dekker Nov 19, 2023

dzenanz Nov 19, 2023

issakomi Nov 20, 2023

N-Dekker commented Nov 20, 2023

dzenanz commented Nov 20, 2023

issakomi commented Nov 21, 2023 •

edited

Loading

N-Dekker left a comment

N-Dekker commented Nov 22, 2023 •

edited

Loading

N-Dekker commented Dec 12, 2023

issakomi commented Dec 18, 2023

N-Dekker commented Dec 18, 2023

N-Dekker commented Dec 18, 2023

issakomi commented Dec 18, 2023 •

edited

Loading

issakomi commented Dec 19, 2023 •

edited

Loading

issakomi commented Dec 19, 2023

N-Dekker commented Dec 19, 2023

issakomi commented Dec 19, 2023 •

edited

Loading

N-Dekker commented Dec 19, 2023

issakomi commented Dec 19, 2023 •

edited

Loading

issakomi commented Dec 19, 2023 •

edited

Loading

ENH: Fixed Coverity 2023.6.2 warnings in itkImageRegion.h #4334

ENH: Fixed Coverity 2023.6.2 warnings in itkImageRegion.h #4334

Conversation

issakomi commented Nov 18, 2023 • edited Loading

N-Dekker commented Nov 19, 2023

N-Dekker Nov 19, 2023

Choose a reason for hiding this comment

dzenanz Nov 19, 2023

Choose a reason for hiding this comment

issakomi Nov 20, 2023

Choose a reason for hiding this comment

N-Dekker commented Nov 20, 2023

dzenanz commented Nov 20, 2023

issakomi commented Nov 21, 2023 • edited Loading

N-Dekker left a comment

Choose a reason for hiding this comment

N-Dekker commented Nov 22, 2023 • edited Loading

N-Dekker commented Dec 12, 2023

issakomi commented Dec 18, 2023

N-Dekker commented Dec 18, 2023

N-Dekker commented Dec 18, 2023

issakomi commented Dec 18, 2023 • edited Loading

issakomi commented Dec 19, 2023 • edited Loading

issakomi commented Dec 19, 2023

N-Dekker commented Dec 19, 2023

issakomi commented Dec 19, 2023 • edited Loading

N-Dekker commented Dec 19, 2023

issakomi commented Dec 19, 2023 • edited Loading

issakomi commented Dec 19, 2023 • edited Loading

issakomi commented Nov 18, 2023 •

edited

Loading

issakomi commented Nov 21, 2023 •

edited

Loading

N-Dekker commented Nov 22, 2023 •

edited

Loading

issakomi commented Dec 18, 2023 •

edited

Loading

issakomi commented Dec 19, 2023 •

edited

Loading

issakomi commented Dec 19, 2023 •

edited

Loading

issakomi commented Dec 19, 2023 •

edited

Loading

issakomi commented Dec 19, 2023 •

edited

Loading