Skip to content

Conversation

@giacomodabisias
Copy link

I think its quite useless and expensive to allocate memory at every registration step.

@fran6co
Copy link
Contributor

fran6co commented Oct 21, 2015

Isn't this PR moving an allocation from heap to stack? I mean, the performance cost shouldn't be around the same?

@giacomodabisias
Copy link
Author

should be less expensive since the compiler can optimize everything, while in the other case we have to make a dynamic allocation.

@kohrt
Copy link
Contributor

kohrt commented Oct 21, 2015

I try to avoid creating large arrays on the stack, because it could lead to problems with the maximum stack size.

@giacomodabisias
Copy link
Author

I think there should be no problems with allocating on stack such a structure since it is not really big:
512_424_4bytes=848Kbyte
Dynamically allocating it is not nice if you use the grabber online at a high rate.

@giacomodabisias
Copy link
Author

What about putting it in the registration class as mutable so the user can decide where to allocate it? :)

@kohrt
Copy link
Contributor

kohrt commented Oct 21, 2015

Right now all the variables in the registration class are only accessed reading from the methods, so you are able to run multiple calls in parallel. If you make it a member, you have to use a mutex to protect it.
Another solution would be to add another pointer to the parameters. If the pointer is null a new array will be allocated and deleted at the end, but if it's not null, it will use that array (and the user has to make sure that it is large enough). Or maybe a reference/pointer to a std::vector.
Anyway, is there a real speedup compared to the current implementation? And what do the others think?

@giacomodabisias
Copy link
Author

I agree 100% with you, but I think it would be nice to be able to decide how to allocate it. I like the second solution you proposed (pointer).

@xlz
Copy link
Member

xlz commented Oct 21, 2015

Allocating 848KB on stack is definitely not OK. MSVC default stack size is 1MB.

I agree allocating 848KB 30 times per second is not optimal. But I don't think registration is performance bottleneck right now.

@giacomodabisias
Copy link
Author

So what about the pointer solution like with the bigdepth frame?

@xlz
Copy link
Member

xlz commented Oct 21, 2015

I guess the solution is changing the interface to let user provide the intermediate data structure.

@giacomodabisias
Copy link
Author

I will do a pull request and then we can discuss about it

@floe
Copy link
Contributor

floe commented Oct 22, 2015

Is there any use for depth_to_c_off outside of apply? I don't think that one malloc/free per frame will make any measurable difference, the size of the allocation doesn't matter.

@giacomodabisias
Copy link
Author

Probably not, but In my opinion its nice to have the user decide it.

@floe
Copy link
Contributor

floe commented Oct 23, 2015

Closing in favor of #441.

@floe floe closed this Oct 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants