emplace_back vs push_back

[JustinPrivitera]

Should we be using emplace_back or push_back for adding items to vectors? For other containers, emplace_back can be slower than regular insertion, but for vectors it is faster.

Should we

1. Go back and hunt down all instances of vector push_back and replace them with emplace_back?
2. Use emplace_back going forward and replace instances of vector push_back when we encounter them?
3. Not use emplace_back at all. 😢

[markcmiller86]

I worry we're asking the wrong question here.

[scratlantis:~/visit/visit] miller86% find src -name '*.C' -exec grep push_back {} \; | wc -l
    6610
[scratlantis:~/visit/visit] miller86% find src -name '*.C' -exec grep emplace_back {} \; | wc -l
       7

The above tells me VisIt is a push_back code base. I am assuming those 7 cases are from changes occurring after taking the C++ class. So, prior to that we were 100% push_back.

Its what we've always used and what I think we should continue to use...that is unless there is some kind of a real issue push_back causes that emplace_back solves. We should not hapazardly mix these.

Don't get me wrong. I value innovation and I want to encourage adoption of new practices. But, IMHO, those new practices need have a reason for being introduced. They need to solve a credible problem. New for the sake of new is not useful. Maybe its shinny. Maybe its modern. Maybe its keeping up with newer language standards. But, IMHO, none of that is relevant if it is not solving a problem.

In the few cases were emplace_back was introduced, do we ever expect the savings in avoiding the extra copy to be noticeable to a human? Maybe a profiler measuring times in microseconds (if the underlying platform's clock even supports that resolution) will notice but I don't think a profiler noticing something humans cannot is a credible reason either.

Were the objects in the cases emplace_back is used so far of such a size that avoiding the copy really matters to our space (or time) performance? If not, its just a different way of doing the same thing we do with push_back.

The three options you offered actually don't cover what I am proposing here so I'll propose a 4th option for your list...

4. Use emplace_back in VisIt only if push_back is not sufficient 😄 👍🏻

[JustinPrivitera]

@markcmiller86

On the release candidate:

Searching 10929 files for "emplace_back"

/usr/WS2/justin/build_containers/3.3RC/visit/src/avt/Expressions/Derivations/avtStrainExpression.C:
  206              if (anyGhost)
  207              {
  208:                 cellsToIgnore.emplace_back(i);
  209                  continue;
  210              }
  ...
  251          } 
  252          else // cell is not a hexhedron also ignore   
  253:             cellsToIgnore.emplace_back(i);
  254          out->SetComponent(i, 0, out2);
  255      }

These ones are just for primitives.

/usr/WS2/justin/build_containers/3.3RC/visit/src/avt/Filters/avtOSPRayCommon.C:
   90        for (unsigned int i = 0; i < size; ++i)
   91        {
   92:         cdata.emplace_back(table[i].R, table[i].G, table[i].B);
   93:         odata.emplace_back(table[i].A);
   94        }
   95        OSPData osp_cdata = ospNewData(cdata.size(), OSP_FLOAT3, cdata.data());
   ..
  243      Light Renderer::AddLight()
  244      {
  245:       core->lightList.emplace_back();
  246        return Light(core->lightList.back());
  247      }
  ...
  249      {
  250        std::vector<OSPLight> osp_light_list;
  251:       for (auto& l : core->lightList) { osp_light_list.emplace_back(*l); }
  252        ospray_rm(core->lightData);
  253        core->lightData = ospNewData(osp_light_list.size(), OSP_OBJECT, 

This looks like an excellent use of emplace_back.

/usr/WS2/justin/build_containers/3.3RC/visit/src/gui/QvisColorTableWindow.C:
  516          for (const auto mapitem : tagList)
  517          {
  518:             tagNames.emplace_back(mapitem.first);
  519:             activeTags.emplace_back(mapitem.second.active);
  520          }
  521          node->AddNode(new DataNode("tagList", tagNames));

The first of these is for strings, and the second is for bools. I believe the second is the one causing the compiler errors so it should be changed anyway. The first is probably fine as is.

8 matches across 3 files

---

On develop:

Searching 10985 files for "emplace_back"

/usr/WS2/justin/build_containers/develop/visit/src/avt/Expressions/Derivations/avtStrainExpression.C:
  206              if (anyGhost)
  207              {
  208:                 cellsToIgnore.emplace_back(i);
  209                  continue;
  210              }
  ...
  251          } 
  252          else // cell is not a hexhedron also ignore   
  253:             cellsToIgnore.emplace_back(i);
  254          out->SetComponent(i, 0, out2);
  255      }

Same as before.

/usr/WS2/justin/build_containers/develop/visit/src/avt/Filters/avtOSPRayCommon.C:
   90        for (unsigned int i = 0; i < size; ++i)
   91        {
   92:         cdata.emplace_back(table[i].R, table[i].G, table[i].B);
   93:         odata.emplace_back(table[i].A);
   94        }
   95        OSPData osp_cdata = ospNewData(cdata.size(), OSP_FLOAT3, cdata.data());
   ..
  243      Light Renderer::AddLight()
  244      {
  245:       core->lightList.emplace_back();
  246        return Light(core->lightList.back());
  247      }
  ...
  249      {
  250        std::vector<OSPLight> osp_light_list;
  251:       for (auto& l : core->lightList) { osp_light_list.emplace_back(*l); }
  252        ospray_rm(core->lightData);
  253        core->lightData = ospNewData(osp_light_list.size(), OSP_OBJECT, 

Same as before.

/usr/WS2/justin/build_containers/develop/visit/src/common/state/ColorControlPointList.C:
 1809  // 
 1810  //    Justin Privitera, Thu Aug 25 15:04:55 PDT 2022
 1811: //    Now takes a const string and uses `emplace_back`.
 1812  //
 1813  // ****************************************************************************
 ....
 1819      if (!HasTag(newtag))
 1820      {
 1821:         tagNames.emplace_back(newtag);
 1822          tagChangesMade = true;
 1823      }

Strings! Not a primitive.

/usr/WS2/justin/build_containers/develop/visit/src/common/state/ColorControlPointList.code:
  868  // 
  869  //    Justin Privitera, Thu Aug 25 15:04:55 PDT 2022
  870: //    Now takes a const string and uses `emplace_back`.
  871  //
  872  // ****************************************************************************
  ...
  878      if (!HasTag(newtag))
  879      {
  880:         tagNames.emplace_back(newtag);
  881          tagChangesMade = true;
  882      }

Same as above.

/usr/WS2/justin/build_containers/develop/visit/src/gui/QvisColorTableWindow.C:
  521          for (const auto& setitem : mapitem.second)
  522              changes << setitem.first << "," << setitem.second << ";";
  523:         retval.emplace_back(changes.str());
  524      }
  525      return retval;

Again, strings.

11 matches across 5 files

[markcmiller86]

There could be some decent benefits -- but we should profile to ask the question.
Or write a small set of cases to gauge.

I'd sure not want anyone to actually spend time doing that simply to debate this point. So, let me save everyone the trouble. With suitably contrived types and/or cases, emplace_back will show a time performance gain and maybe even some space performance gain, if your timer has sufficient resolution 😉. Certainly, it will be no slower. So what?

Ultimately, for me, this isn't about emplace_back. Its about staying on the beaten path and wondering off only when truly necessary. That the path is beaten is reason enough for me to stay on it.

All that said, I can learn to read emplace_back as well as I've learned push_back. And, I think we've got more important things to spend our time on.

[markcmiller86]

Nothing is ever simple in C++ land :-(

It looks like others agree on that point.

[JustinPrivitera]

It looks like others agree on that point.

Similar to what the linked page says, I have been saying since the start that emplace_back is almost always advantageous for vectors. With other containers, mileage may vary. It depends on the container as to whether or not you want to construct something in place or create something and move it in. For example, with a dictionary, emplace_back is almost always wrong because you never want to create an object in place in a dictionary, because you should check first if the object is already in the dictionary. emplace_back will create the object and then check. Hence why emplace_back can be slower in that case.

[JustinPrivitera]

Ultimately, for me, this isn't about emplace_back. Its about staying on the beaten path and wondering off only when truly necessary. That the path is beaten is reason enough for me to stay on it.

Unfortunately, we may not come to an agreement. I don't quite understand the beaten path argument. To me, it seems more like a case of using a different feature (only when applicable) to accomplish something in a different, better, way. Each have their uses and situations in which they are more advantageous. I don't really see a reason not to use it when and where it is applicable.

However, I can try to limit my usage only to situations where I have definitive reason to believe that using emplacement with vectors will be advantageous.

[cyrush]

It looks like the current emplace_back issue is related to bools + boolVector aka std::vector<bool>.

If you weren't aware -- std::vector<bool> has been a source of evil and confusion since the beginning of C++ time.

For this case, since std::vector<bool> is specialized (as part of the standard, or maybe just in most implementations?) there are issues with C++11 support, and C++14 sorted that out, see:

https://stackoverflow.com/questions/44971360/stdvectorboolemplace-in-c11

I don't see the exact mention of missing support alluded to on the current cppref (I think the page changed since the stack overflow post was made), but I believe this is why Justin's reasonable attempt was blocked by evil.

[JustinPrivitera]

Ahhhh I forgot! When I wrote that code I wasn't thinking about how bool vectors are inherently scary.
As a side note, I believe future versions of the color table window on develop move away from using bool vectors mostly.
I should have read that post you linked more closely.

This seems to be a great example of a case where emplace_back is the wrong option, at least with the version of cpp/compiler we use right now.

Doesn't exactly help my case... 🙃

Anyhow, the current emplace_backs causing compile issues ought to be changed to push_backs.

[markcmiller86]

FYI...it looks like there are other ways to cause compile issues as well.

[JustinPrivitera]

FYI...it looks like there are other ways to cause compile issues as well.

I believe the reason for this is because emplace_back uses perfect forwarding, which is an advantage. It's less ambiguous than push_back. It's like how C++ has 4 functions for doing casting, each of which are less ambiguous than the classic C-style and classic C++-style casts. This particular case looks rather odd, but in general emplace_back can protect you from doing things you don't mean to do.