[PATCH] Performance and style improvements in scriptinterface

[Jonas Platte]

I just looked through the scriptinterface folder to get into the C++ / JS interaction stuff.

This patch just improves the performance a little bit and should also bring a better code style to some parts of the code. It doesn't change any interfaces or the behaviour of the code.

[sanderd17]

What's the advantage of using std::string() over ""? (just want to learn)

[Jonas Platte]

std::string() is the standard constructor which by definition creates an empty string. If you use "", you are calling std::string(const char* s) with s = "" which potentually takes longer because it probably starts a strcpy or something like that to copy s.

Though I'm pretty sure it has an impact on the performance, the improvement is probably so tiny that it can be ignored in most cases :D

[alpha123]

Most (all?) std::string implementations are copy-on-write, which will (probably) never kick in for the empty string. So it has 0 performance impact in return for being longer code.

[Jonas Platte]

How can a string be copy-on-write? It makes sense for a constant string, but how does it work that when you initialize a std::string with a c-string and then edit the c-string, the std::string doesn't change?

[alpha123]

I've reviewed this and am not going to commit it. There are no actual performance improvements. Most of the changes are worthless, although you did spot a number of inconsistencies that deserve to be fixed.

• Everything (except possibly the change pointed out in my 3rd bullet point) before line 71 of DebuggingServer.cpp is useless, although I guess calling string#empty is arguably more semantically correct than comparing against the empty string, they're both equally clear and != "" is shorter.
• Thanks for fixing the inconsistent whitespace on lines 71 and 115.
• Regarding the changes from << stream.str() to << stream, I suppose that's as clear and shorter so that wouldn't be a bad change to make.
• The "" to std::string() conversion is pointless everywhere.
• Indentation fixes are always a good thing (on lines 441-442).
• I guess loop-local variables are good style on line 533.
• Unlike Yves said on the forums, ENSURE does actually run in both debug and release builds, however I think it was clearer the old way (assigning the result of the function to a variable and ensuring that was 0).
• ThreadDebugger.cpp:171-176 is indeed somewhat more readable your way.
• I'm not going to comment on the rest of the indentation fixes, string#empty calls, or replacing "" with std::string as you already know how I feel about those.
• I guess calling string#clear is ah... clearer... than assigning the string to "" at line 681.
• I dislike the movement of the declaration of fp from line 721 to 735. It was marginally more readable before the change, since it was declared with the other JSStackFrame and other local variables.
• You fixed the spelling on line 770 but neglected to fix the punctuation?
• Yay for const for CompareScriptInterfacePtr.
• On ThreadDebugger.h:30,45,53 your changes are good but IIRC elsewhere in the codebase we use a space between a set of empty braces.

TL;DR - No performance change, mix of good and bad style changes.

[alpha123]

How can a string be copy-on-write? It makes sense for a constant string, but how does it work that when you initialize a std::string with a c-string and then edit the c-string, the std::string doesn't change?

Ignore that since C++11 disallows COW std::string implementations anyway.

String literals are implicitly const (well actually I'm not sure about that, but if you write to them expect bad things) so I think that's not a problem here, and would use a COW string in non-C++11 implementations. I'm not actually a C++ programmer however, so I could very well be wrong.

[Jonas Platte]

Replying to alpha123:

How can a string be copy-on-write? It makes sense for a constant string, but how does it work that when you initialize a std::string with a c-string and then edit the c-string, the std::string doesn't change?

Ignore that since C++11 disallows COW std::string implementations anyway.

String literals are implicitly const (well actually I'm not sure about that, but if you write to them expect bad things) so I think that's not a problem here, and would use a COW string in non-C++11 implementations. I'm not actually a C++ programmer however, so I could very well be wrong.

Okay, since you now seem to be unsure if my changes from "" to std::string() have a (positive) impact on performance, and I don't know the details either, I'll just test it. Write a test program that creates a fixed number of strings in a loop and outputting how long it took afterwards using the time stuff from the STL (chrono). I won't have both methods in the same program, so that they can't impact each other, and I'll run them multiple times to be sure.

Thanks a lot for the feedback about the rest of my changes. I'll update the patch as soon as the string construction performance is clear.

[Yves]

I wouldn't spend too much time optimizing ThreadDebugger and DebuggingServer because I will have to change most of that code anyway for the new Spidermonkey version.

[alpha123]

Meh, I'm actually wrong about the copy-on-write stuff, that only kicks in in some circumstances involving std::string's copy constructor.

I would think the compiler would optimize it to be the same regardless. Make sure you compile with -O3.

[Jorma Rebane]

Replying to alpha123:

Most (all?) std::string implementations are copy-on-write, which will (probably) never kick in for the empty string. So it has 0 performance impact in return for being longer code.

That would be correct if this was C# or Java, but in fact in C++ this is not completely true and COW only applies on std::string to std::string assignment/init. In the case of std::string, the matter is more complex and the Wikipedia article with examples of COW is only correct when it comes to GCC: ​http://en.wikipedia.org/wiki/Copy-on-write

If you take a look at VC++ std::string, it doesn't have any COW semantics. GCC std::string has just an extra pointer indirection, but behind the scenes the real string object is implemented roughly as:

class string {
    size_t m_Length;
    char* m_Pointer;
    char m_Buffer[20];
};

Any strings with length less than 20 get put into a local buffer to avoid dynamic memory overhead of small strings. This is actually a really good optimization for the strings. Initializing strings looks roughly like:

string() : m_Length(0), m_Pointer(NULL) {
    m_Buffer[0] = '\0';
}
string(const char* str) {
    m_Length = strlen(str);
    if (m_Length < 20) {
        memcpy(m_Buffer, str, m_Length + 1);
        m_Pointer = NULL;
    } else {
        m_Pointer = (char*)malloc(m_Length + 1);
        memcpy(m_Pointer, str, m_Length + 1);
    }
}

At any rate, you can see the difference between ("") and () init quite clearly. And no, you can't optimize this in some magical way, because std::string is not a built-in class. It's just part of STL that is provided with the C++ compiler for convenience.

Also, in our case we need code that runs fast on both Linux and Windows. That means we have to take into account the underlying differences of the STL, too. For example, std::vector is ridiculously slower in VC++ Debug compared to GCC.

C++11 solves a lot of this mess by introducing Move operators and constructors, but that's a topic for another time.

Actual patch review: But yeah, these are micro-optimizations, but it's good for someone to slowly improve the quality of the coding style in general. As for ENSURE macro - your code is correct unlike what other people have commented already. ENSURE is applied both in debug and release, so everything looks right.

At any rate, the patch itself conforms to good C++ style (i.e. using string.empty() instead of string == ""), so after a quick test this can be committed.

[Jonas Platte]

Replying to alpha123:

Meh, I'm actually wrong about the copy-on-write stuff, that only kicks in in some circumstances involving std::string's copy constructor.

I would think the compiler would optimize it to be the same regardless. Make sure you compile with -O3.

Well, I found out something really weird. When I compile the program with -O0 (was default), both methods take about 1,78 seconds to create (and delete) 1000000000 strings on my machine. But when I enable the optimization (I tried -O2 and -O3), the method with "" took more than 6 seconds (the other one still takes 1,78 seconds)! Try it yourself, this is the code I used: ​http://pastebin.com/2wypxhsv (I have gcc 4.8.1, but that shouldn't matter that much)

Now that this is clear, I'll just update my patch corresponding to your other feedback.

[Jorma Rebane]

P.S. alpha123, I know you hate C++, but this review is kind of harsh, really. Regarding good C++ practices, our previous code for these (albeit insignificant) files wasn't good C++.

Using string::clear() and string::empty() is all good C++. Doing string != "" is bad. Doing string != CStr("") is even worse (we actually have a lot of that).

This is not Java :) Perhaps you should take some C++ courses and brush up on your general knowledge on C++? In fact you said it yourself in IRC a while back: the obvious way in C++ is usually the wrong way.

[alpha123]

Replying to RedFox:

Replying to alpha123:

Most (all?) std::string implementations are copy-on-write, which will (probably) never kick in for the empty string. So it has 0 performance impact in return for being longer code.

That would be correct if this was C# or Java, but in fact in C++ this is not completely true and COW only applies on std::string to std::string assignment/init. In the case of std::string, the matter is more complex and the Wikipedia article with examples of COW is only correct when it comes to GCC: ​http://en.wikipedia.org/wiki/Copy-on-write

Yeah, I found out I was udderly wrong about the COW stuff. Thanks for the explanation.

At any rate, you can see the difference between ("") and () init quite clearly.

memcpying the empty string should be ridiculously cheap.

And no, you can't optimize this in some magical way, because std::string is not a built-in class. It's just part of STL that is provided with the C++ compiler for convenience.

I refuse to believe a C++ compiler couldn't optimize that, or at least that there's not a special case for the empty string in the STL implementation. GHC does all sorts of optimizations with standard library functions. C++ compilers would have to be really dumb to not be able to figure out that something is an STL class.

Actual patch review: But yeah, these are micro-optimizations, but it's good for someone to slowly improve the quality of the coding style in general. As for ENSURE macro - your code is correct unlike what other people have commented already. ENSURE is applied both in debug and release, so everything looks right.

At any rate, the patch itself conforms to good C++ style (i.e. using string.empty() instead of string == ""), so after a quick test this can be committed.

The "micro optimizations" make the code longer in return for not being any faster. Also note this code isn't a bottleneck or performance intensive at all, plus is going to be thrown away in a matter of weeks or months.

The problem with ENSURE is that while it isn't a mistake, it looks like one, even to an experienced C++ programmer like Yves. It doesn't help that the docs in lib/debug.h are rather ambiguous.

[Jonas Platte]

Replying to alpha123:

• You fixed the spelling on line 770 but neglected to fix the punctuation?

Well now that you reminded me of that again: I can just fix that one issue with the code instead of fixing the spelling :D

Just wait for my next update on the patch :)

[Jonas Platte]

Replying to alpha123:

The "micro optimizations" make the code longer in return for not being any faster. Also note this code isn't a bottleneck or performance intensive at all, plus is going to be thrown away in a matter of weeks or months.

As my test program shows, those optimizations have an impact (see comment 11). You could be right about other people throwing it away, but I plan to do some more contributions on 0ad so I can be there to tell people why they should do it this way :P

The problem with ENSURE is that while it isn't a mistake, it looks like one, even to an experienced C++ programmer like Yves. It doesn't help that the docs in lib/debug.h are rather ambiguous.

I don't really know what this is about, but I'll just revert my change to that part of the code

[alpha123]

Replying to RedFox:

P.S. alpha123, I know you hate C++, but this review is kind of harsh, really. Regarding good C++ practices, our previous code for these (albeit insignificant) files wasn't good C++.

Using string::clear() and string::empty() is all good C++.

Like I said, that stuff is arguably good changes. I liked the usage of clear().

Doing string != "" is bad. [citation needed]

It's not terrible though. Hey, if you think empty is better, I'm not against it.

This is not Java :) Perhaps you should take some C++ courses and brush up on your general knowledge on C++? In fact you said it yourself in IRC a while back: the obvious way in C++ is usually the wrong way.

I have actually been learning more C++ for the lobby. I'm not too terrible at it (well OK, I don't claim to know the STL particularly well at all). I do know when something needs optimization and when something doesn't. I also know when some code looks good and some code looks less good and claims to be 0.0001ms faster. That's bad.

[Jonas Platte]

Replying to jP_wanN:

Replying to alpha123:

• You fixed the spelling on line 770 but neglected to fix the punctuation?

Well now that you reminded me of that again: I can just fix that one issue with the code instead of fixing the spelling :D

Just wait for my next update on the patch :)

Okay, cancelled that. It seems those variables are used in more than one function (thought they were just used in their namespace, which would kind of make sense), so I can't make them function-static.

[alpha123]

Replying to jP_wanN:

Replying to alpha123:

Meh, I'm actually wrong about the copy-on-write stuff, that only kicks in in some circumstances involving std::string's copy constructor.

I would think the compiler would optimize it to be the same regardless. Make sure you compile with -O3.

Well, I found out something really weird. When I compile the program with -O0 (was default), both methods take about 1,78 seconds to create (and delete) 1000000000 strings on my machine. But when I enable the optimization (I tried -O2 and -O3), the method with "" took more than 6 seconds (the other one still takes 1,78 seconds)! Try it yourself, this is the code I used: ​http://pastebin.com/2wypxhsv (I have gcc 4.8.1, but that shouldn't matter that much)

Now that this is clear, I'll just update my patch corresponding to your other feedback.

Even if this were true, these kinds of micro optimizations don't matter at all, especially on this kind of code which isn't in any way, shape, or form a bottleneck.

std::string() seems to be roughly 1.5x faster on clang with no optimizations (I was actually surprised it was this big), and with -O3 it optimizes the loop away for the std::string() version completely.

[Jonas Platte]

So now this is the final version of the patch...

[alpha123]

Re-reviewed. Only complaint besides that I don't like the std::string stuff is the inconsistent style of empty braces between ThreadDebugger.cpp:177-178 and ThreadDebugger.h:30,45,53.

Oh, and I'm not sure if "getters / setters" is more correct than "getters/setters" in the comments. In fact I think without spaces is more common (e.g. I see and/or a lot more than and / or).

[Jonas Platte]

As it's nothing important, I reverted my changes to the comments with '/'. I also fixed the style of the empty braces.

I edited the patch by hand, but it should be usable.

[Jonas Platte]

Sorry, I think it doesn't work like that. I'll redo it this evening...

[Jonas Platte]

Now it should really be exactly how I wanted it to be. Sorry for the many renewals of the patch.

[Jorma Rebane]

You can test if a stream is empty with this code:

if (stream.tellp() == 0)
    // stream is empty

Everything else looks ok.

[Jonas Platte]

Updated the stringstream emptiness testing.

[Jorma Rebane]

Looks like stream.tellp() requires some extra casting, otherwise it has some operator ambiguity issues:

if (stream.tellp() != 0) // error: 2 overloads have similar conversions

if ((int)stream.tellp() != 0) // ok

Sorry for not including that cast. Do you plan to add any more changes to the style patch? If you're done, just add "patch review" into keywords.

[Jonas Platte]

Fixed the string emptiness testing, added "patch review" to keywords.

[Jorma Rebane]

Changes committed to r13865.

[leper]

You can use SpecialCommands to close the ticket when committing.

[historic_bruno]

These changes broke the JS debugger. See #2175

[ben]

In 13973:

Fixes JS debugger, fixes VS 2013 build error, re-fixes #2127, #2175