Jump to content

  • Log In with Google      Sign In   
  • Create Account

Ideas for how to improve code


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
59 replies to this topic

#41 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 06 May 2012 - 03:37 PM

Here's a counterpoint for you: Boost is maintained by some of the leading experts in C++ in the world.


Over the years I've received a lot of advice from Boost authors. Some of it has been directed to me and some of it has come from reading the Boost mailing lists and applying something. Originally the only way to use the CMW was via this web interface. (That interface is no longer active/live.) Someone on a Boost list encouraged me to develop an interface that could be easily integrated into build environments. The CMW is a new Boost Serialization library. It has support for some Boost libraries that the serialization library that's in Boost doesn't have. Originally I had very little open-source code... today I have over 5,000 lines of it and growing. I've gotten a lot of helpful input from many highly regarded C++ people: Bjarne Stroustrup, James Kanze, Boost authors, people here and on other forums. And Telastyn noted that the CMW is also free.

Your project is maintained solely by you, and having existed for over 9 years, still has glaring code quality issues by even your own admission.

What do you think people are liable to trust more?

I'm one of the maintainers of the CMW, but not the only one. I don't argue that the CMW has no room for improvement. That's why I started the thread. The point is the serialization library that's in Boost is losing momentum. I wouldn't be working on this if I didn't believe there was a changing of the guard taking place. The older approach worked OK for years, but now there's a better (more portable and sustainable) way to do it. We're out here proving it to people. The remarks about how the serialization library in Boost hasn't been well maintained is getting at how sustainable that model is. A business model matters if you want a helpful partner that will be here for as long as your project lasts. Is Google going to release a version of Google Docs that you install locally? Is Microsoft going to stop developing Office 365? Posted Image

Edited by wood_brian, 06 May 2012 - 03:39 PM.


Sponsor:

#42 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 06 May 2012 - 04:44 PM

Your project is maintained solely by you, and having existed for over 9 years, still has glaring code quality issues by even your own admission.


A couple more thoughts on this. I agree with those who say, "C++ isn't very good, but it is better than the alternatives." And I think Stroustrup made the comment, "now we have to hurry up and fix everything" when C++ started getting popular. I don't think C++ has fixed everything yet, but it has improved a lot over the years. I believe the CMW is also on the right path -- getting better over time.

#43 e‍dd   Members   -  Reputation: 2105

Like
0Likes
Like

Posted 07 May 2012 - 08:35 AM

Something is wrong with getaddrinfo if it doesn't set res and it doesn't fail.

Assertions check and document your understanding and assumptions. So this is a good place for one, I'd say.

#44 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 13 May 2012 - 01:45 PM

Recently I noticed the last line in an if matched the last line in an else here:

void
cmwAmbassador::mediateResponse (transactions_t::iterator itr)
{
  bool safe_sending = true;
  try {
	localsendbuf.sock_ = itr->sock;
	bool reqResult;
	remoteMessages.Receive(cmwBuf, reqResult);

	if (reqResult) {
	  changeDirectory(itr->path.c_str());
	  ::std::vector<File> outputFiles;
	  remoteMessages.Receive(cmwBuf, outputFiles);

	  save_lastruntime(itr->filename, itr->time_in_seconds);
	  safe_sending = false;
	  localMessages.Marshal(localsendbuf, true);
	  localsendbuf.Flush();
	} else {
	  ::std::string errorMsg;
	  remoteMessages.Receive(cmwBuf, errorMsg);
	  safe_sending = false;
	  localMessages.Marshal(localsendbuf, false, errorMsg);
	  localsendbuf.Flush();
	}
  } catch(::std::exception const& ex) {
#ifdef SYSLOG_AVAILABLE
	syslog(LOG_ERR, "mediateResponse: %s", ex.what());
#endif
	if (safe_sending) {
	  localMessages.Marshal(localsendbuf, false, ex.what());
	  localsendbuf.Flush();
	}
  }
}

... and I factored it out. I felt certain the new version would be the same size as it is such a simple thing. I was surprised when the size of the executable decreased by 88 bytes using gcc 4.7.0 20120314. Using gcc 4.8.0 20120429 the size decreased by 56 bytes. I'm using -O3 in both cases. This isn't some tricky thing and these are some recent compilers. I thought twice about even doing this as I was so sure the compiler would be able to catch it.

Edited by wood_brian, 13 May 2012 - 01:46 PM.


#45 Antheus   Members   -  Reputation: 2397

Like
1Likes
Like

Posted 13 May 2012 - 01:56 PM

HP drivers I use are 370MB. Radeon drivers are 250MB.

I have 16GB of RAM and 3TB of disk.

Just saying.

For machines where 30 bytes matters, C++ is absolutely the wrong choice, precisely due to the hidden magic and "bloat", compared to C.

#46 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 20 May 2012 - 03:29 PM

For machines where 30 bytes matters, C++ is absolutely the wrong choice, precisely due to the hidden magic and "bloat", compared to C.


Someone in another forum pointed out the local objects being destructed -- not deep magic, but that explains part of it. I've now made those static local objects. After doing that the size of the executable is 28 bytes larger when the line is repeated in both branches. Perhaps that's all that can be done and I'll just chalk the 28 bytes up to a weakness in the compiler.

void
cmwAmbassador::mediateResponse (transactions_t::iterator itr)
{
  bool safe_sending = true;
  try {
		localsendbuf.sock_ = itr->sock;
		bool reqResult;
		remoteMessages.Receive(cmwBuf, reqResult);

		if (reqResult) {
		  changeDirectory(itr->path.c_str());
		  static ::std::vector<File> outputFiles;
		  outputFiles.clear();
		  remoteMessages.Receive(cmwBuf, outputFiles);

		  save_lastruntime(itr->filename, itr->time_in_seconds);
		  safe_sending = false;
		  localMessages.Marshal(localsendbuf, true);
		  localsendbuf.Flush();
		} else {
		  static ::std::string errorMsg;
		  remoteMessages.Receive(cmwBuf, errorMsg);
		  safe_sending = false;
		  localMessages.Marshal(localsendbuf, false, errorMsg);
		  localsendbuf.Flush();
		}
  } catch(::std::exception const& ex) {
#ifdef SYSLOG_AVAILABLE
		syslog(LOG_ERR, "mediateResponse: %s", ex.what());
#endif
		if (safe_sending) {
		  localMessages.Marshal(localsendbuf, false, ex.what());
		  localsendbuf.Flush();
		}
  }
}

Edited by wood_brian, 20 May 2012 - 03:29 PM.


#47 Cornstalks   Crossbones+   -  Reputation: 6991

Like
1Likes
Like

Posted 20 May 2012 - 03:59 PM


For machines where 30 bytes matters, C++ is absolutely the wrong choice, precisely due to the hidden magic and "bloat", compared to C.


Someone in another forum pointed out the local objects being destructed -- not deep magic, but that explains part of it. I've now made those static local objects. After doing that the size of the executable is 28 bytes larger when the line is repeated in both branches. Perhaps that's all that can be done and I'll just chalk the 28 bytes up to a weakness in the compiler.

Honestly, I'm not sure why you even care about 28 bytes. Improving code and minimizing executable size don't really go hand in hand, especially if it's a matter of just a few bytes. If you're really, seriously, critically concerned about shaving off a few bytes, you'd be pulling out ugly code hacks anyway.

Edited by Cornstalks, 20 May 2012 - 04:00 PM.

[ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

#48 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 20 May 2012 - 04:07 PM

  • try/catch(...) anywhere but at an extremely high level always worries me. Any instance of throw; immediately worries me even more. The combination is egregious. You should really reconsider your approach to exceptions. Also, I haven't confirmed this deeply, but I suspect that most of your code is highly unsafe in the presence of thrown exceptions.


I grepped through Boost 1.49 and try/catch(...) and throw; are both used in hundreds of places. You've mentioned Boost in this thread in positive terms so am pointing out that others are using the same functionality in a similar context.

#49 Cornstalks   Crossbones+   -  Reputation: 6991

Like
0Likes
Like

Posted 20 May 2012 - 04:13 PM

  • try/catch(...) anywhere but at an extremely high level always worries me. Any instance of throw; immediately worries me even more. The combination is egregious. You should really reconsider your approach to exceptions. Also, I haven't confirmed this deeply, but I suspect that most of your code is highly unsafe in the presence of thrown exceptions.


I grepped through Boost 1.49 and try/catch(...) and throw; are both used in hundreds of places. You've mentioned Boost in this thread in positive terms so am pointing out that others are using the same functionality in a similar context.

Boost can be configured to use no exceptions... See boost/serialization/throw_exception.hpp, seeing as that seems to be a relevant comparison to your project.
[ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

#50 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 20 May 2012 - 04:18 PM

Honestly, I'm not sure why you even care about 28 bytes. Improving code and minimizing executable size don't really go hand in hand, especially if it's a matter of just a few bytes. If you're really, seriously, critically concerned about shaving off a few bytes, you'd be pulling out ugly code hacks anyway.


Well, I'm happy about how this has turned out -- noticing that I can make the variables static. I'm not very concerned now about the 28 bytes, but if I hadn't brought it up, I probably wouldn't have thought about making the variables static.

#51 rip-off   Moderators   -  Reputation: 8726

Like
1Likes
Like

Posted 20 May 2012 - 04:31 PM

I don't think it is a good idea to use "static" when the end result is a couple of bytes saved or lost. It changes the semantics of your program, and particularly has implications for how you can support threading. The cleanup code for these static objects is not free either, the compiler must ensure they are properly destroyed at the end of the program.

#52 Cornstalks   Crossbones+   -  Reputation: 6991

Like
1Likes
Like

Posted 20 May 2012 - 05:04 PM

Personally, I think that's a horrible use of static. It:
  • Makes your code non-thread safe
  • Is non-idiomatic
  • Is a bit more convoluted
  • Is not clear why you made it static (seriously, to save a few bytes?)
  • Is more prone to errors (what if you forget to clear() one day when making something static?)
  • Reeks of code-smell

I am in no way trying to be demeaning when I say this, but: if you really think that's a good use of static, then you can be sure I have no confidence in your code or your library.
[ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

#53 ApochPiQ   Moderators   -  Reputation: 16396

Like
1Likes
Like

Posted 20 May 2012 - 06:51 PM

  • try/catch(...) anywhere but at an extremely high level always worries me. Any instance of throw; immediately worries me even more. The combination is egregious. You should really reconsider your approach to exceptions. Also, I haven't confirmed this deeply, but I suspect that most of your code is highly unsafe in the presence of thrown exceptions.


I grepped through Boost 1.49 and try/catch(...) and throw; are both used in hundreds of places. You've mentioned Boost in this thread in positive terms so am pointing out that others are using the same functionality in a similar context.



I said that it worried me, not that it was wrong outright. I respect the Boost usage of exceptions because I know that the library authors are generally understanding of the semantics and subtle sticking points of exception usage in C++. I trust the patterns they use in those circumstances.

Your usage, on the other hand, strikes me as a stab in the dark attempt at "bulletproofing" your code by duct taping cardboard to it. The usage I've seen in your code base does not suggest to me that you are handling exceptions correctly most of the time, nor does it suggest to me that you had good cause to do what you did. It smells like superstitious programming and that always sets of alarm bells for me.


If you can't tell the difference between the Boost exception semantics and your own, I'd really question if you know the C++ language well enough to be writing such a low-level library for it.

#54 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 21 May 2012 - 10:56 PM

I don't think it is a good idea to use "static" when the end result is a couple of bytes saved or lost. It changes the semantics of your program, and particularly has implications for how you can support threading. The cleanup code for these static objects is not free either, the compiler must ensure they are properly destroyed at the end of the program.


Reusing the same vector and string each time the function runs takes pressure off the heap. The process here is a server so the cleanup code for the static objects executes rarely.

#55 wood_brian   Banned   -  Reputation: 197

Like
1Likes
Like

Posted 21 May 2012 - 11:12 PM

Your usage, on the other hand, strikes me as a stab in the dark attempt at "bulletproofing" your code by duct taping cardboard to it. The usage I've seen in your code base does not suggest to me that you are handling exceptions correctly most of the time, nor does it suggest to me that you had good cause to do what you did. It smells like superstitious programming and that always sets of alarm bells for me.


It would help if you could be more specific here. I don't consider myself to be an exceptions expert so it wouldn't surprise me if you have a point.

#56 ApochPiQ   Moderators   -  Reputation: 16396

Like
1Likes
Like

Posted 22 May 2012 - 11:16 AM

Reusing the same vector and string each time the function runs takes pressure off the heap. The process here is a server so the cleanup code for the static objects executes rarely.


This is true if and only if you don't cause a realloc of the heap object by using it again. Also, unless you have profiler evidence that you are under serious heap pressure to begin with, this is a premature optimization and probably not worth making. It is better for your code to be clear than to be perfectly optimal in 99% of the cases. Only when you truly have numerical evidence that you need speed (or storage space, or whatever) should you be worried about optimization.


It would help if you could be more specific here. I don't consider myself to be an exceptions expert so it wouldn't surprise me if you have a point.


Here's one example:

cmw_request
getRequest (uint8_t byteOrder
            , sock_type sock)
{
  static ReceiveBufferTCP<SameFormat> localbuf(4096);
#ifdef CMW_ENDIAN_BIG
  static ReceiveBufferTCP<LeastSignificantFirst> otherlocalbuf(4096);
#else
  static ReceiveBufferTCP<MostSignificantFirst> otherlocalbuf(4096);
#endif

  uint8_t clientFormat;
  Read(sock, &clientFormat, 1);
  try {
    if (clientFormat == byteOrder) {
      localbuf.sock_ = sock;
      while (true) {
        localbuf.BulkRead();
        if (localbuf.GotPacket()) 
          return cmw_request(localbuf);
      }
    }
    otherlocalbuf.sock_ = sock;
    while (true) {
      otherlocalbuf.BulkRead();
      if (otherlocalbuf.GotPacket()) 
        return cmw_request(otherlocalbuf);
    }
  } catch (::std::exception const& ex) {
    clientFormat == byteOrder ? localbuf.Reset() : otherlocalbuf.Reset();
    throw;
  }
}

Because localbuf is a static, throwing an exception out of here can leave it in a potentially invalid (or maybe even undefined) state. The only way I could conclusively prove that this is safe would be to walk through every single line of code and every invoked function to see if you have any situations where the exception can cause a stack unwind that corrupts the buffer's state. I don't have time to do that unfortunately, but that's the kind of thing that concerns me when I see exceptions in C++.

cmwAmbassador::mediateResponse is another case where this can get messy. If an exception is thrown at just the right time, you can leave your logic in an indeterminate state, which is bad. It seems to me - and again I unfortunately don't have time to prove whether or not this is the case - that you could potentially do disastrous things in there with that safe_sending flag; it just feels to me like it's unsafe and risky. I might be totally wrong; again I'm not trying to say there's a definite bug or flaw in the code. It just has a bad aroma to it, and if you can write your code to be clearer about how it works and less likely to arouse that kind of suspicion, I think it would be much improved for the effort.

Another area where your code concerns me is your socket handling. Once again I don't have time to prove this conclusively, but it seems like you are not adhering to RAII of your sockets. This means that a thrown exception can leave a socket in an indeterminate state - very, very, very bad for a server. Consider cmwAmbassador::reset - what if one of the functions you call throws? Now your sockets are leaked and possibly in invalid states.

This is the rabbit hole of exception usage in C++. It's extremely hard to get right and I hope I don't come off as dismissive or negative when I say you probably aren't there yet; to be frank I'm not that great with C++ exception safety myself. I do respect that you're willing to learn about it, though!

#57 Cornstalks   Crossbones+   -  Reputation: 6991

Like
0Likes
Like

Posted 22 May 2012 - 12:43 PM


I don't think it is a good idea to use "static" when the end result is a couple of bytes saved or lost. It changes the semantics of your program, and particularly has implications for how you can support threading. The cleanup code for these static objects is not free either, the compiler must ensure they are properly destroyed at the end of the program.


Reusing the same vector and string each time the function runs takes pressure off the heap. The process here is a server so the cleanup code for the static objects executes rarely.

If profiling shows the heap to be a significant problem in these functions, why don't you use a smarter, custom allocator?
[ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]

#58 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 23 May 2012 - 09:07 PM

It is better for your code to be clear than to be perfectly optimal in 99% of the cases. Only when you truly have numerical evidence that you need speed (or storage space, or whatever) should you be worried about optimization.


Fine, but am not convinced this approach is less clear. It has one additional line -- a call to clear(). On the other hand, not using statics here gets the compiler adding hidden code to be sure destructors are called when the scope ends or in the event of an exception.

Several years ago I started a thread in the Gamedev networking forum about how I was thinking about switching from a 2-tier model to a 3-tier architecture. At the time my primary reason for thinking it was a good idea was efficiency/security related -- the middle tier would be a server and would check passwords much less frequently than under the 2-tier model. Iirc hplus pointed out how the 3-tier model would also be beneficial from an administrative/firewall perspective -- only the machine the middle tier runs on would need attention from a network admin. I didn't have profiling evidence at the time that moving to a 3-tier model would be helpful, but believe it was the right decision.


Because localbuf is a static, throwing an exception out of here can leave it in a potentially invalid (or maybe even undefined) state. The only way I could conclusively prove that this is safe would be to walk through every single line of code and every invoked function to see if you have any situations where the exception can cause a stack unwind that corrupts the buffer's state. I don't have time to do that unfortunately, but that's the kind of thing that concerns me when I see exceptions in C++.


These statics are used for the same reason as those we've been discussing -- to minimize the dependency on the heap. The calls to Reset() in the catch clause are meant to address what you are talking about here. If they don't, Reset might have a bug.

Edited by wood_brian, 24 May 2012 - 07:31 PM.


#59 JohnnyCode   Members   -  Reputation: 292

Like
0Likes
Like

Posted 24 May 2012 - 04:15 PM

first thing to ask if you write a code/

- do you want to compromise a code that serves as an abstraction to other programers?
example 1 - System.Net.Security.SslStream
.... amazing axample making tls encripted communication upon initiated certifcate = a simple stream.

- do you want to utilyze an application that performs fast and stable?
.... leave all frameworks and reduce all instructions to native and responsible device communications
example : DirectX

So, if you are abstracting rather large interactive happening to ease developers, or you want to rule all OS resource at its deepest level. Think!
code is mad

you will never get code that is:
readable-
standard-
fast-
effective-
least computaion demanding-

.... at once

#60 wood_brian   Banned   -  Reputation: 197

Like
0Likes
Like

Posted 14 June 2012 - 01:57 PM

This is more important
http://lists.boost.o...10/05/58858.php

That hasn't been addressed as of Boost 1.49 and I haven't seen any indication it will be in 1.50. As I note on this page, the boost-based program is more than 2.4 times slower than the equivalent Ebenezer version.


I have a few updates on things discussed in this thread. Boost 1.50 beta is now out and I've updated the performance page that I mentioned above -- the performance of the serialization library in Boost hasn't improved as far as taking advantage of std::move.

While I flick through the code again, I notice that you tend not to initialize variables that are immediately passed as references to functions that fill them in. I always think it's a good idea to initialize everything, as any place where you introduce the possibility for the behaviour of a program to differ between debug and optimized builds is asking for trouble;



Edd wrote that. It took me a long time to figure it out, but I've reworked some interfaces. Previously I had a functions like this:

template <class B>
void boolReceive(B& buf, bool& bl);

Now I've changed to this:
template <class B>
bool boolGet(B& buf);

I also managed to eliminate a try/catch block from one of the lower-level functions.

Thanks again for the ideas in this thread. I haven't addressed everything but am happy with the results so far.

Yesterday I read this
http://lists.boost.o...12/06/74828.php

which says:

Asio is "friendly" enough to define WIN32_LEAN_AND_MEAN for you, which pretty much breaks any other consumer of Windows.h.

I didn't know that was a no-no... I also define that in a header. I commented out the define, but the code wouldn't build without it. I'm wondering if I need to do something differently regarding WIN32_LEAN_AND_MEAN.




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS