Sign in to follow this  
AndyEsser

std::vector allocation failure

Recommended Posts

AndyEsser    394
Hi All,

I've recently switched from doing [i]std::vector::push_back()[/i] to doing [i]std::vector::resize()[/i] to avoid having to use it in a loop, and just pre-allocating the number of indices I need.

This has all worked fine recently, however I've just tried it on another [i]std::vector[/i] with a custom Class as the [i]std::vector[/i] type, and I'm getting problems.

Below is the offending line. If I change it back to a [i]push_back()[/i] style then it performs the first allocation, and fails on the second.

[i][b]Elements[/b][/i] is [i]std::vector[/i] itself with the type shown below, which contains the [i][b]svMaterials[/b][/i] std::vector.

[source]
struct CHIMERA_MODEL_ELEMENT
{
CHIMERA_MODEL_ELEMENT_HEADER ElementHeader;
std::vector<CHIMERA_MODEL_VERTEX> svVertices;
std::vector<CHIMERA_MODEL_TVERTEX> svTVertices;

std::vector<CHIMERA_MODEL_FACE> svFaces;
std::vector<CHIMERA_MODEL_MATERIAL_DESCRIPTOR> svMaterialsDesc;
std::vector<Chimera::Texture> svMaterials;
};
[/source]


[source]
Elements[elem].svMaterials.resize(Elements[elem].ElementHeader.sNumMaterials);
[/source]

Even if I stick it into a[i] try()...catch()....[/i] block it's always fails.

Can anyone see something obvious? I know it's probably a really stupid mistake.

I've been over the previous lines of code to ensure it's not some other memory related issue that's just showing up on this line, but can't see anything.

Many thanks,

Andy

Share this post


Link to post
Share on other sites
Wooh    1088
Do all the types in CHIMERA_MODEL_ELEMENT and in the vectors implement proper copy operations (copy constructor, copy assignment operator, destructor)?
If not, that can be be your problem.

Share this post


Link to post
Share on other sites
AndyEsser    394
That may well be it - all those of other Vectors (except for svMaterials) are just plain Structs - whereas Chimera::Texture is a class.

Cheers for that. I will go and implement a Copy constructor.

Will let you know if I continue to have problems.

Share this post


Link to post
Share on other sites
the_edd    2109
[quote name='AndyEsser' timestamp='1304774813' post='4807665']
That may well be it - all those of other Vectors (except for svMaterials) are just plain Structs - whereas Chimera::Texture is a class.
[/quote]
FYI, there's very little difference between a class and a struct. The only differences are to do with access specifiers.

[quote]
Cheers for that. I will go and implement a Copy constructor.
[/quote]
The [url="http://drdobbs.com/184401400"]rule of three[/url] may be relevant, which is why Wooh mentioned the assignment operator and destructor too.

Incidentally, what kind of failure are you experiencing? Have you debugged the code to ensure the amount of memory being asked for isn't crazy-large?

Share this post


Link to post
Share on other sites
AndyEsser    394
Yep - I was already going to abide by the Rule of Three, but thanks for the link.

The debugger returns this error (on the return line of [i][b]__tmainCRTStartup[/b][/i] within [i][b]crtexe.c[/b][/i])

[code]
First-chance exception at 0x7c90e4ff in Chimera.System.exe: 0xC0000008: An invalid handle was specified.
[/code]

And it's just a size of 2 that's trying to be 'resized', which equates to 108 bytes.

Share this post


Link to post
Share on other sites
AndyEsser    394
Ok so I've implemented an Assignment Operator and a Copy Constructor (as shown below). I already a destructor for the class. But I'm still continuing to get the same problem.

[source]
Texture::Texture(const Texture& cpTexture)
{
Texture::textureID = 0;
Texture::NeedsUpdate = false;

Texture::hDC = CreateCompatibleDC(0);
}

Texture& Texture::operator=(const Texture& asTexture)
{
Texture::textureID = 0;
NeedsUpdate = false;

hDC = CreateCompatibleDC(0);

return *this;
}
[/source]

Could this have something to do with the fact that this class contains [b]Critical Sections[/b]? Given that the error is talking about an invalid handle?

Share this post


Link to post
Share on other sites
rip-off    10976
Those implementations are almost certainly incorrect. Why are you deleting the mutex in the copy constructor? Your assignment operator is empty.

You probably want your Texture class to be "noncopyable". Sometimes implementing the copy and assignment operators is not only difficult, but is actually a really bad idea for performance. So you instead disable the compiler generated versions of these functions using something like boost::noncopyable or just declaring the functions private and not providing an implementation.

You would then store them in a container of smart pointers, for example using std::shared_ptr.

Share this post


Link to post
Share on other sites
AndyEsser    394
Hi rip-off.

I updated the code sections after I made the post because I'd updated the Copy and Assignment constructors.

I'll try making it non-copyable, and then using std::shared_ptr.

Share this post


Link to post
Share on other sites
Matias Goldberg    9573
Hey, one important element. Resize != reserve.

For efficiency often you want to reserve, then push_back.

By calling resize, you shouldn't call push_back because you would be duplicating your entries. And if you don't, you're calling the constructor for each iteration when calling resize, which if you're later going to overwrite the value, it's a performance hit. Only in a very few cases the compiler may optimize that.

Share this post


Link to post
Share on other sites
AndyEsser    394
Hey - I don't use Resize & push_back at the same time - always only one or the other. I also I've yet to have to Resize multiple times - I tend to just Resize once, at the loading of a Model for the number of materials that the Model requires. After that it should basically never have to be touched again.

Share this post


Link to post
Share on other sites
Wooh    1088
If you are using c++0x features the vector will try to move the object instead of copying. So in that case you can to implement move constructor and move assignment operator instead. If you don't need shared ownership you can use std::unique_ptr instead of shared_ptr.

Share this post


Link to post
Share on other sites
Antheus    2409
Windows handles are not suitable for STL semantics.

You save a handful of cycles by reserving a vector then waste *trillions* needed to make each copy. Life cycle as required by std::vector is incompatible with heavy hardware resource allocation performed by each of calls above.

WinAPI resources are intended for kernel and OS. You don't just copy or create them just to be fancy. If a resource is created it will exist precisely for as long as it's needed. If for some reason it needs to be duplicated, then and only then it is, but in order to have two copies, not to make an overcomplicated move or swap. Never just for the sake of it to constrain to some foreign API.


Even more, critical sections or other synchronization primitives are incompatible with rule of three, since they cannot be properly implemented inside a destructor, meaning it can fail. Even though such constructs are often exposed using RAII, the semantics they represent cannot be properly implemented to comply with requirements of C++ object life cycle. They are, by definition, non-copyable.

1) remove all threading related constructs - synchronization is performed elsewhere (that means, classes like this cannot own a member synchronization primitive unless they are non-copyable, meaning they cannot be put into containers)
2) anything that allocates hardware or OS resources needs to be heap allocated and passed around via pointer, since its life-cycle does not match that of C++ scopes and auto-allocation

Share this post


Link to post
Share on other sites
Matias Goldberg    9573
[quote name='AndyEsser' timestamp='1304782742' post='4807710']
Hey - I don't use Resize & push_back at the same time - always only one or the other. I also I've yet to have to Resize multiple times - I tend to just Resize once, at the loading of a Model for the number of materials that the Model requires. After that it should basically never have to be touched again.
[/quote]

Cool, I didn't want to sound rude. I also wanted to establish the point that reserve then push_back may be faster than resize() then vector[i] = x depending the case ;)


Cheers

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this