Jump to content

  • Log In with Google      Sign In   
  • Create Account


std::vector allocation failure


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
13 replies to this topic

#1 AndyEsser   GDNet+   -  Reputation: 375

Like
0Likes
Like

Posted 07 May 2011 - 06:52 AM

Hi All,

I've recently switched from doing std::vector::push_back() to doing std::vector::resize() 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 std::vector with a custom Class as the std::vector type, and I'm getting problems.

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

Elements is std::vector itself with the type shown below, which contains the svMaterials std::vector.

	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;
	};



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


Even if I stick it into a try()...catch().... 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

Sponsor:

#2 Wooh   Members   -  Reputation: 550

Like
1Likes
Like

Posted 07 May 2011 - 07:26 AM

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.

#3 AndyEsser   GDNet+   -  Reputation: 375

Like
0Likes
Like

Posted 07 May 2011 - 07:26 AM

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.

#4 e‍dd   Members   -  Reputation: 2105

Like
1Likes
Like

Posted 07 May 2011 - 07:53 AM

That may well be it - all those of other Vectors (except for svMaterials) are just plain Structs - whereas Chimera::Texture is a class.

FYI, there's very little difference between a class and a struct. The only differences are to do with access specifiers.

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

The rule of three 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?

#5 AndyEsser   GDNet+   -  Reputation: 375

Like
0Likes
Like

Posted 07 May 2011 - 08:01 AM

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 __tmainCRTStartup within crtexe.c)

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

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

#6 AndyEsser   GDNet+   -  Reputation: 375

Like
0Likes
Like

Posted 07 May 2011 - 08:59 AM

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.

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;
}


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

#7 rip-off   Moderators   -  Reputation: 7642

Like
2Likes
Like

Posted 07 May 2011 - 09:20 AM

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.

#8 AndyEsser   GDNet+   -  Reputation: 375

Like
0Likes
Like

Posted 07 May 2011 - 09:22 AM

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.

#9 Matias Goldberg   Crossbones+   -  Reputation: 3007

Like
2Likes
Like

Posted 07 May 2011 - 09:36 AM

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.



#10 AndyEsser   GDNet+   -  Reputation: 375

Like
0Likes
Like

Posted 07 May 2011 - 09:39 AM

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.

#11 Wooh   Members   -  Reputation: 550

Like
1Likes
Like

Posted 07 May 2011 - 09:43 AM

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.

#12 Antheus   Members   -  Reputation: 2393

Like
2Likes
Like

Posted 07 May 2011 - 10:31 AM

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

#13 AndyEsser   GDNet+   -  Reputation: 375

Like
1Likes
Like

Posted 07 May 2011 - 10:49 AM

Hi Antheus.

Thanks for those points. Guess I've got a bit of a redesign to do then.

Many thanks.

#14 Matias Goldberg   Crossbones+   -  Reputation: 3007

Like
1Likes
Like

Posted 07 May 2011 - 12:18 PM

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.


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






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