Ideas for how to improve code
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.
Personally, I think that's a horrible use of static. It:
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.
- 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 [font=courier new,courier,monospace]clear()[/font] 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.
[quote name='ApochPiQ' timestamp='1336085463' post='4937241']
- 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.
[/quote]
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.
Author
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.
Author
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.
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!
[quote name='rip-off' timestamp='1337553067' post='4941750']
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.
[/quote]
If profiling shows the heap to be a significant problem in these functions, why don't you use a smarter, custom allocator?
Author
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++.
[/quote]
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.
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
- 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
Author
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.
[color=#000000]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;
[color=#000000][/quote]
[color=#000000]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.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
