4.4 release work?

[safemode]

Since the creation of the 4.4 branch, I haven't noticed _anything_ going on in it. Is it just assumed that the only further changes to the release will be in the data4.x dir? Are there no bugfixes and changes occuring in the svn head that should be backported to the 4.4 branch? Should we not be testing the 4.4 branch to try and expose any issues prior to release? We should be releasing this code in only a week or so. Probably should have brought this up sooner but I was too busy chasing moby.


So what's the deal with 4.4? is the code frozen save data4.x or should we be backporting bugfixes, or submiting them to someone for approval to be backported?

[ace123]

No, all the developers are using HEAD, so no one is bothering to update the branch.
I was using the branch while the UnitIterator memory corruption issues happened too often to be able to test anything... but that was only for a short period.

So I think when newer revisions actually go to the 4.4 branch, it will end up being more of a tag since it will (hopefully) be synced up with HEAD upon release.

My biggest concern code-wise is about the memory corruption problems happening in head (a whole lot of glibc double free, std::bad_alloc, and friends).
It happens pretty often so I'm going to run valgrind when I have time to see what the real reason is.

I have a suspicion that my music loader thread is at fault since it uses VSFileSystem and strings and other possibly dangerous elements, and I wrote it with very little prior multi threading experience.
The new version of the music system (including the thread) is IMO necessary for release, since the old music system was not working for most people, so I'm going to have to spend some time fixing that.

There is still a chance that these crashes might in fact be related to the unit iterators... I'll see what valgrind reports. Either way, I hope it's something simple.

[charlieg]

IMHO all recent HEAD work should be in the next release, including safemode's stuff. This will it will get much better testing and should any bugs get fixed you can quickly do 0.4.4.x bugfix releases.

Or you could just call this version 0.5 and start being a bit more aggressive with version numbering.

[safemode]

I've valgrinded my new unitcollection and it has so far been better at cleaning itself up than the old Unitcollection by far. For instance, deleting a unitcollection in the old version left all the nodes hanging apparently ( as far as valgrind shows me). Also for some reason I'd get random nodes being left in the list even though unitcollection thinks it's empty with the old version.

Valgrinding an entire vegastrike load would be pretty fun. But i dont see any of the crashing or corruption at least with the latest svn that you were talking about. Not even with the music stuff. I'll valgrind total_war_python with the modification. If there is a bug in unitcollection, that's probably the best way to expose it since it hits unitcollection more than any other object.

[safemode]

i loaded up a non-total_war mission. just to quick test before work.

==12357== Mismatched free() / delete / delete []
==12357== at 0x4A1FFF8: operator delete[](void*) (vg_replace_malloc.c:256)
==12357== by 0x759AFE: SaveGame::ParseSaveGame(

==12357== ERROR SUMMARY: 408768 errors from 137 contexts (suppressed: 0 from 0)
==12357== malloc/free: in use at exit: 286,734,169 bytes in 1,996,064 blocks.
==12357== malloc/free: 7,278,207 allocs, 5,282,143 frees, 2,529,510,288 bytes allocated.
==12357== For counts of detected errors, rerun with: -v
==12357== searching for pointers to 1,996,064 not-freed blocks.
==12357== checked 261,202,696 bytes.
==12357==
==12357== LEAK SUMMARY:
==12357== definitely lost: 5,963,077 bytes in 86,384 blocks.
==12357== possibly lost: 17,742,679 bytes in 234,072 blocks.
==12357== still reachable: 263,028,413 bytes in 1,675,608 blocks.
==12357== suppressed: 0 bytes in 0 blocks.


I didn't have time this morning to do any more. I know not a single mention of UnitCollection in the entire output though. (i'm also not playing with music enabled, though sound is). Didn't see nothing about the music player either.

[chuck_starchaser]

I've seen cases of concrete class inheritance where the base class' destructor is non-virtual. I pointed it out to Klauss and Hellcat that there was a likely case of slicing; and got no feedback whatsoever from them; that was like two years ago, and I don't remember what classes these were. The safest thing would be to declare destructors virtual in every class, regardless of inheritance; but otherwise at least all inherited, non-abstract classes should have virtual dtors.

I've also seen "delete this", which is not necessarily likely to cause leaks, per se, unless it's inside some picky conditional; but it's really BAD programming; like, what if someone instances the class on the stack?; what about stack temporaries created by the compiler? Chances are "delete this" would cause so many headaches as to be put into some conditional that later proves too conditional, and causes a leak.

[safemode]

I dont know if any classes that have virtual functions have a non-virtual destructor.... I know I removed UnitIterator's virtual destructor. The thing is, the derived classes dont ever reference those derived classes by the base type. So, that is safe.


I just wanted to add before my boss sees me. The vast majority of errors in my post above come from libraries using uninitialized variables in conditionals. Though, there were definitely malloc/free issues. I'll run a more thorogh test tonight.

[chuck_starchaser]

OT: I have a deep dislike of concrete inheritance in general. There's a little known language called "Sather" that prohibits concrete inheritance. You can have deep class hierarchies, as long as all classes are abstract except the leafs, at the bottom of the hierarchy. I've stuck to that philosophy in C++ as a matter of discipline and it has paid off. Just asking the question "why do I need to have a concrete parent class here?" usually points out a weakness in the design.
/OT

I dont know if any classes that have virtual functions have a non-virtual destructor..

Virtual functions are not necessary to necessitate a virtual destructor. You could have

Code: Select all

class A
{
    int foo;
public:
    A();
    ~A();
    int get_foo();
};
class B : public A
{
    double bar;
public:
    B();
    ~B();
    double get_bar();
}
A *an_A = new B;
delete A; //get your slice

Needs virtual dtor even though no other functions need to be virtual.

I know I removed UnitIterator's virtual destructor. The thing is, the derived classes dont ever reference those derived classes by the base type. So, that is safe.

I'm not familiar with it, but I'd say the question to ask is not so much whether derived class objects are accessed polymorphically at present, as whether they could be in the future. Some other programmer might assume the dtor is virtual and they may be well justified to make the assumption...
Furthermore, polymorphism has ways to creep in unnoticed; you might have an operator defined somewhere in another class perhaps, that takes a reference to the base class and, if you forgot to define that operator for the derived class, when you pass the the derived object it gets converted to a base class pointer or reference. Sure, it would take a good dose of spaghetti code to delete an object from an operator in some other class, but I've seen worse... "delete this" being a prime example.

[safemode]

Oh, i know full well the possibility is there .. but that is true of any class at all. I could decide to inherit any arbitary class and have an issue now because it's destructor is not virtual. So I really dont buy that argument. There are perfectly valid reasons to inherit a class but not reference it by the base class ever, and thus not need a virtual destructor. Most notably function reuse and logistic grouping of objects.

What would be an argument is that there are no defined api's or reference material about such classes, or any classes.

The idea here is to enforce a strict api or procedure for using objects to minimize overhead. Why have a class like unitIterator (which did support use of polymorphism) handle the overhead of a vtable pointer when there is not one instance of requiring that functionality at all in the entire source? The classes need to be as fast as possible, not compromise speed just for the sake of possible flexibility. Now, if the flexibility is warranted, that's different. But in many cases, these are "what if's" not "it is".

[chuck_starchaser]

Point well taken. I hardly ever have a virtual destructor in my code (or virtual anything unless pure virtual). But then again, I avoid concrete inheritance like the plague, and I avoid concrete inheritance with size augmentation like the plague squared; but I've seen both of these things done in the vs engine. Just thought I'd call your attention to it, since you seem to have found leaks.

[safemode]

here's the output of the svn head valgrind run on a normal mission. no jumping (the game was really extremely slow). cycled through targets, shot missiles, shot guns changed views and flew around a bit.

http://signal-lost.homeip.net/files/vs_valgrind_out.txt

[chuck_starchaser]

Never used valgrind; my only experience with a leak analyzer was a commercial product, years ago. Any idea where in the code that's all happening?
I see pretty much near the end of that file,

Code: Select all

.......................................
Loading a starsystem
Loading Star System Crucible/Everett
 Next To: Crucible/Enyo
 Next To: Crucible/Stirling
 Next To: Crucible/Cardell
 Next To: Crucible/17-ar
 Next To: Redemption/Quetal
 Next To: Redemption/Ynyeeldah==28446== 
...................................

Which, if I were to take a guess, means that the entire file represents only initializations time, rather than play time. Play time would be more valuable info, as undeallocated things that are only allocated once are kind of irrelevant, as they'll be deallocated when the app exits anyways; but continuous leaks during play would be the killers.

[safemode]

playtime did not expose many issues. The overall majority of output is uniinitialized variables in library routines vegastrike is linked to. You have to grep through for free / delete messages or any bt starting with a vegastrike function, rather than a library.

I'll have to hit total_war tonight to see if runtime shows any issues. It takes about a half hour to get to the actual game. And my machine is fast.

It's really too bad the python code cant execute in a separate thread. Lots of cpu is spent in the python code rather than the C++ vegastrike code.

[chuck_starchaser]

Hellcat was keen on moving AI from Python to a dll/so. I offered to help with the work, but I can't do it alone as I don't know Python at all, and I don't know enough about the engine to even find where anything happens. I can write C++, but that's just about it; as of presently, I couldn't even test what I write, since the current vegastrike svn doesn't even compile correctly for me.
(I have Visual 8, but without service pack 1; and I get problems with SDL when compiling for debug. Compiling for release compiles and links, but at runtime it tries to access a missing function in sdl.dll. Never got past this problem, even with Hellcat's help.)

[safemode]

Gotta love windows dev environments.

[charlieg]

Perhaps it's worth trialing Python speed-up hacks like Psyco:
http://psyco.sourceforge.net/

There was another but I can't remember it's name.

[safemode]

I think for now, we're going to focus on just hand optimizing the actual python ....rather than get another project to depend on


Getting the AI out of python though would probably be a good idea. Sounds like a post 4.4 issue though to me. We're trying to see what is to be done for the 4.4 release sometime within the next couple weeks

[chuck_starchaser]

Oh, i know full well the possibility is there .. but that is true of any class at all. I could decide to inherit any arbitary class and have an issue now because it's destructor is not virtual. So I really dont buy that argument. There are perfectly valid reasons to inherit a class but not reference it by the base class ever, and thus not need a virtual destructor. Most notably function reuse and logistic grouping of objects.

What would be an argument is that there are no defined api's or reference material about such classes, or any classes.

The idea here is to enforce a strict api or procedure for using objects to minimize overhead. Why have a class like unitIterator (which did support use of polymorphism) handle the overhead of a vtable pointer when there is not one instance of requiring that functionality at all in the entire source? The classes need to be as fast as possible, not compromise speed just for the sake of possible flexibility. Now, if the flexibility is warranted, that's different. But in many cases, these are "what if's" not "it is".

Just wanted to make a point: vtables are only generated in the cases of concrete inheritance. Inheritance of abstract classes does not generate vtables, or rather, any compiler worth the bits it's made of will know to optimize away the vtable where there's only one concrete entry to it. And what I would advocate is abstaining from concrete class inheritance as a matter of discipline.

The cases you mention, "Most notably function reuse and logistic grouping of objects" are also perfect candidates for use of abstract inheritance. So you would still make their destructors virtual, and probably pure virtual.

So, you're right in that "There are perfectly valid reasons to inherit a class", but I've yet to see ONE valid reason to inherit a concrete class (and MANY valid reasons NOT to, such as slicing on polymorphic assignment, etceteras). And, by avoiding concrete inheritance, one also avoids vtables.

Side story:
What little fame the language Sather enjoys is mostly due to its being the fastest executable generating object oriented language in the planet; makes faster code than Fortran, in many cases. May sound unbelievable, in light of the fact that it supports 3 types of (multiple) inheritance, and yet it is also considered THE most air-tight of all compiled OO languages, in terms of type safety; even tighter than Eiffel (co-variant outputs and contra-variant inputs). And one of the ways in which it differs from many other OO languages is in that it does not allow concrete inheritance, thereby bypassing the need for vtables altogether.

CORRECTION:

For non-Sather compilers to be able to optimize away vtables completely, one may need to go two steps further than avoiding concrete inheritance:

First step is to adopt the following discipline also:
Functions in the abstract base class are either defined AND non-virtual AND never overridden AND public; or they are always overridden and therefore pure virtual (and private). Nothing in-between; --or if you do have to have a non-pure virtual function then it should be neither public nor private, but protected (See NOTES.)

Second step is to refactor pure virtual, overridden functions away from polymorphism and towards expicit overloading by use of the Visitor pattern. This generates much leaner and faster code than vtables and substitutes for their need; and it also makes the sources easier to understand and maintain (one ends up with a table of function overloads, where one can see, at a glance, how a given function differs for each of the classes in a hierarchy, rather than have to look up that function in the implementations of each of the classes).

At the end of this refactoring process, you'd have a hierarchy of abstract classes of any given complexity, concrete classes that inherit any which way from this abstract soup --but never from another concrete class; Visitors for all polymorphic behaviors, and inheritance of non-virtual functions that is never overridden (straight code re-use). The only virutal functions inherited, in the final analysis, are pure virtual destructors.

May sound like a lot of work, but each of these disciplines have fringe benefits, beauties and hidden treasures to offer, --i.e. are Good disciplines; as opposed to the alternative discipline... avoiding polymorphic USE... which is unenforceable, error-prone, un-rewarding, inelegant, Bad, and even contradictory... (using an OO language only to avoid polymorphism? ).

NOTE: One might ask "and how do you ensure that a function is never overridden?" Well, there are OO languages that can explicitly prevent a function being overridden in derived classes; C++ is not one of them. However, one discipline that is advocated in C++ is that of ONLY overriding private or protected functions. In fact, this is an "idiom": making a private function virtual denotes an intent for the derived class to substitute a behavior; whereas making a protected function virtual denotes an intent for the derived class to access the parent class function from within the overriding function so as to add to, or decorate, the original function. Making a public function virtual is meaningless and deprecated. So, within the constraints of this idiom, one can "ensure" that a function is never overridden by making it public.

NOTE2:
Heresy! How do you avoid public virtual functions?
Easily:

Code: Select all

class Base
{
    int count;
public:
    void foo(){ foo_(); } //may throw
    int bar() const { return bar_(); }; //nothrow
protected:
    virtual void foo_() { ++count; } //may throw
private:
    virtual int bar_() const = 0; //nothrow
};

[safemode]

There are really only a few examples of inheritance in the game.

Unit
UnitCollection::UnitIterator

In fact, that's all i can think of off the top of my head. Unit Is inherited by GameUnit. un_iter is inherited by PythonUnitIter and Planet::PlanetIter. In the case of Unit, who really knows wtf is going on there. It's really out of control both in size, function, and purpose. In the case of un_iter, the things that talk to un_iter are un_iter's, The things that talk to PythonUnIter all refer to it as PythonUnIter. The weird and strange PlanetIter is only referred to by PlanetIter.

There is one area where the above isn't true. ActiveIters is a vector of un_iter pointers to all the iterators that have been associated with the collection. So a python,planet,plain un_iter will all be looked at with un_iter pointers. Now, nothing is destroyed by UnitCollection in this manner. So the destructors will never be touched by pointers to un_iter.

So I really dont see what the big problem is. Can you name some classes specifically that are not including a virtual destructor but are being destroyed by pointers to the base class?

[chuck_starchaser]

Okay, if inheritance is hardly ever used, then disregard my previous post. No, I don't have a specific case to point out; it was years ago that I found something looking really dangerous: A class inheriting from another concrete class, with size augmentation (adding member variables), used polymorphically, and yet the base class didn't have a virtual destructor. It gave me shivers at the time and I posted in this forum about it, but I don't remember what the classes were.

It's a pity, though, that inheritance is not widely used; it's a great way to organize code. Take "unit" for instance. There's a gazillion categories of units, from ships to stations to planets to jump points to sub-units... I would have one abstract_unit base class containing all the variables and code that pertains to every type of unit, then special attribute abstract classes like tractorable, then create a family of concrete classes that multiple-inherit from abstract_unit and one or more attribute classes. And each concrete unit type would use its own kind of "units.csv", with only those columns that pertain to it. This would be a breath of fresh air even in the sense that right now units.csv is a bloated file, both vertically and horizontally.

What having a single "unit" class is doing is basically implementing polymorphism at the source level, with flags that have to be checked to find out its specific type, which is error-prone, inelegant, and defeats the purpose of using an OO language, IMO.

[safemode]

I already proposed an arganization of Unit which, instead of using a purely heirarchal polymorphic method, utilized separate member classes that could be created / deleted on demand inside a base unit class that got templated to the various types of GameUnits.

See, not a lot of Unit yields itself to polymorphism when you really think out the organization. Member classes that handle the various different sections of Unit make much more sense. We could new and delete the graphics subsection whenever a unit is outside of the visible range of the player. We could decide not to create a cockpit object for units without one. Etc etc. Not only does this have a real gameplay benefit by utilizing less resources when they're not needed but it allows us to put those subclasses into their own files and deal with them individually.

[chuck_starchaser]

Ah, excellent! Sounds like you got a real good handle on unit; kudos!

[ace123]

Okay, to resolve the issue of the branch, I have talked to some of the developers, and everyone is using trunk now.

However, I continue to find a few crashes involving deleted Unit's. To test if this behaviour is from the new UnitCollection, I had to find a way to put the old one back.
So what I ended up with is oldcollection.h and oldcollection.cpp, included in #ifdef's from collection.h and collection.cpp (default to USE_OLD_COLLECTION for now)

Now, the two iterator interfaces are compatible, so it should be possible to switch back and forth between them. I hope to be able to release with the new collection as it should be faster, but this way it leaves us with other options aside from branching the entire source like it was originally when it was really bad.

[safemode]

You know, one thing i dont check that i really should have is if python or even C++ holds multiple copies of the same iterator and tries removing it. The first one will be fine, it'll remove, and if there are more than 4 holding an iterator to that collection, it wont change the values of the other iterators. So it'll nullify that iterator's value and push it to the deleted unit list.

Then if a second copy of that iterator simply calls remove, without advancing then it'll try doing the exact same thing. This wouldn't be a segfaultable problem except that i call *Unit->UnRef() and that wont be very cool when *Unit == NULL.

Note, this would only happen if the number of simultaneous iterators to a collection is > 3.

I fixed this by making a conditional at the top of UnitCollection::erase

if(!(*it2){
++it2;
return;
}


That may fix your crashing problem.

the reason i didn't consider that to be a problem before was because i thought python would advance the iterator when it became killed, not ask to remove it. Though there may be some code somewhere that doesn't advance, and when there are a lot of other iterators active, this could cause a problem.

[safemode]

Well, it should be known by now that DDS loading has become a feature for the next release. All that was left to make it really in there was software decompression.

I'm currently just finishing up the final touches to the software decompresser. A working revision should be committed to both my branch and head today.

The problem was i had assumed i had to free buffer, before replacing it, but the aux_texture code frees it's buffer right after calling gl's transfer texture function, so i can just re-assign buffer to my own uncompressed memory and free it at the end of the function.


So now the move can definitely go forward to distributing dds textures with the next release to speed things up drastically.