Small code review like avoiding redundant objects copy. #1247

Equod · 2018-06-12T17:32:39Z

PARTIAL CODE REVIEW

Basically the main change is adding Handlers to the pipeline as "make_shared" pointer.

jcague

I like this PR a lot. Thanks a lot! My only concern is the use of std::move() for paths that don't need optimization yet, but it's just a matter of simplicity and trying to avoid future bugs. I'd like to know your opinion.

jcague · 2018-06-13T10:36:15Z

erizo/src/erizo/WebRtcConnection.cpp

@@ -737,7 +737,7 @@ void WebRtcConnection::syncWrite(std::shared_ptr<DataPacket> packet) {
 }

 void WebRtcConnection::setTransport(std::shared_ptr<Transport> transport) {  // Only for Testing purposes
-  video_transport_ = transport;
+  video_transport_ = std::move(transport);


In general I'm a bit reluctant to use std::move() in places that aren't hot paths because it's too easy to introduce bugs if we try to use the pointer later in the code, am I right?

Coping the shared pointer basically causes reference counter increment, function move instead doesn't affect reference counter at all because it simply transfers its ownership to another object.
Please consider the following code:

#include <iostream> #include <memory> class A { public: std::string some_usefull_string; }; void do_stuff_with_copied_shared_ptr(std::shared_ptr<A> ptr) { std::cout << "before coping use_count is " << ptr.use_count() << std::endl; std::shared_ptr<A> local_ptr = ptr; std::cout << "after coping use_count is " << ptr.use_count() << std::endl; } void do_stuff_with_moved_shared_ptr(std::shared_ptr<A> ptr) { std::cout << "before moving use_count is " << ptr.use_count() << std::endl; std::shared_ptr<A> local_ptr = std::move(ptr); std::cout << "after moving ptr use_count is " << ptr.use_count() << std::endl; if(!ptr) std::cout << "because ptr is not valid any more" << std::endl; if(local_ptr) std::cout << "but local_ptr is valid" << std::endl; std::cout << "and local_ptr use_count is " << local_ptr.use_count() << std::endl; } int main() { std::shared_ptr<A> ptr = std::make_shared<A>(); std::cout << "after construction use_count is " << ptr.use_count() << std::endl; do_stuff_with_copied_shared_ptr(ptr); do_stuff_with_moved_shared_ptr(ptr); std::cout << "before leaving use_count is " << ptr.use_count() << std::endl; return 0; }

Obviously std::move() invalidates the object passed (ptr in this case), so you should be aware of this, but you can use "local_ptr" which is always valid. And this is the best practice in small functions like setters because the reference counter is atomic and changing it requires more time then a for standard variable. It's just a little performance improvement, so feel you free to use it in the way you like. :)
For more info please take a look at this topic on stackoverflow.

Yes, I know how it works, my point is whether the possibility to introduce bugs if we use an invalidated pointer when updating this code by accident worths the code optimization. We always have to consider that specially in OS projects we can receive PRs updating this code and it could be difficult to detect this kind of issues during a review. But I also see that setters are a reasonable place where we can use std::move.

I perfectly agree. So if it's OK for you, in my PRs I'll move(shared_ptr) only in one line setters.

jcague · 2018-06-13T10:38:12Z

erizo/src/erizo/DtlsTransport.cpp

@@ -279,10 +279,10 @@ void DtlsTransport::onHandshakeCompleted(DtlsSocketContext *ctx, std::string cli
  boost::mutex::scoped_lock lock(sessionMutex_);
  std::string temp;

-  if (rtp_timeout_checker_.get() != NULL) {
+  if (rtp_timeout_checker_) {


jcague · 2018-06-13T10:38:17Z

erizo/src/erizo/DtlsTransport.cpp

  ELOG_WARN("%s message: Handshake failed, transportName:%s, openSSLerror: %s",
            toLog(), transport_name.c_str(), error.c_str());
  running_ = false;
  updateTransportState(TRANSPORT_FAILED);
 }

-std::string DtlsTransport::getMyFingerprint() {
+std::string DtlsTransport::getMyFingerprint() const {


jcague · 2018-06-13T10:38:22Z

erizo/src/erizo/DtlsTransport.cpp

@@ -316,14 +316,14 @@ void DtlsTransport::onHandshakeCompleted(DtlsSocketContext *ctx, std::string cli
  }
 }

-void DtlsTransport::onHandshakeFailed(DtlsSocketContext *ctx, const std::string error) {
+void DtlsTransport::onHandshakeFailed(DtlsSocketContext *ctx, const std::string& error) {


jcague · 2018-06-13T10:38:31Z

erizo/src/erizo/MediaStream.cpp

-  pipeline_->addFront(PacketCodecParser());
-
-  pipeline_->addFront(PacketWriter(this));
+  pipeline_->addFront(std::make_shared<PacketReader>(this));


jcague · 2018-06-13T10:38:44Z

erizo/src/erizo/WebRtcConnection.cpp

@@ -455,7 +455,7 @@ std::string WebRtcConnection::getJSONCandidate(const std::string& mid, const std

  std::ostringstream theString;
  theString << "{";
-  for (std::map<std::string, std::string>::iterator it = object.begin(); it != object.end(); ++it) {
+  for (std::map<std::string, std::string>::const_iterator it = object.begin(); it != object.end(); ++it) {


jcague · 2018-06-13T10:38:52Z

erizo/src/erizo/dtls/DtlsClient.cpp

      mSocket->getMyCertFingerprint(fprint);
-      return std::string(fprint, strlen(fprint));
+      return std::string(fprint);


jcague · 2018-06-13T10:39:00Z

erizo/src/erizo/logger.h

@@ -35,21 +35,21 @@ class LogContext {
  LogContext() : context_log_{""} {
  }

-  virtual ~LogContext() {}
+  virtual ~LogContext() = default;


jcague · 2018-06-13T10:39:07Z

erizo/src/erizo/media/ExternalOutput.cpp

@@ -316,10 +316,10 @@ void ExternalOutput::initializePipeline() {
  pipeline_->addService(quality_manager_);
  pipeline_->addService(stats_);

-  pipeline_->addFront(LayerBitrateCalculationHandler());
-  pipeline_->addFront(QualityFilterHandler());
+  pipeline_->addFront(std::make_shared<LayerBitrateCalculationHandler>());


jcague · 2018-06-13T11:07:59Z

btw, tests failed but it seems like docker registry could have failed, can you rebuild it to check if tests are green?

EDIT: I fixed the issue with CircleCI in another PR and I merged it here to see if it now passes all the tests

Equod added 2 commits June 12, 2018 19:22

Small code review like avoiding redundant objects copy.

b0848dc

Fixed lint problem.

6d2fccd

jcague approved these changes Jun 13, 2018

View reviewed changes

Merge branch 'master' into code_review

9e2ae5d

jcague merged commit da05c61 into lynckia:master Jun 13, 2018

Equod deleted the code_review branch June 13, 2018 15:40

zevarito pushed a commit to zevarito/licode that referenced this pull request Jul 4, 2018

Small code review like avoiding redundant objects copy. (lynckia#1247)

013ca00

Arri98 pushed a commit to Arri98/licode that referenced this pull request Apr 6, 2021

Small code review like avoiding redundant objects copy. (lynckia#1247)

4a5a373

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small code review like avoiding redundant objects copy. #1247

Small code review like avoiding redundant objects copy. #1247

Equod commented Jun 12, 2018

jcague left a comment

jcague Jun 13, 2018

Equod Jun 13, 2018 •

edited

Loading

jcague Jun 13, 2018

Equod Jun 13, 2018

jcague Jun 13, 2018

jcague Jun 13, 2018

jcague Jun 13, 2018

jcague Jun 13, 2018

jcague Jun 13, 2018

jcague Jun 13, 2018

jcague Jun 13, 2018

jcague Jun 13, 2018

jcague commented Jun 13, 2018 •

edited

Loading

Small code review like avoiding redundant objects copy. #1247

Small code review like avoiding redundant objects copy. #1247

Conversation

Equod commented Jun 12, 2018

jcague left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Equod Jun 13, 2018 • 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

jcague commented Jun 13, 2018 • edited Loading

Equod Jun 13, 2018 •

edited

Loading

jcague commented Jun 13, 2018 •

edited

Loading