gnuradio icon indicating copy to clipboard operation
gnuradio copied to clipboard

Circular Buffers - Size Optimization

Open NotTheMattThatYouKnow opened this issue 7 years ago • 7 comments

GR ring buffers are sometimes artificially inflating their required size during allocation.
This is because the allocated buffer sizes are forced to be: an integer multiple of granularity an integer multiple of (nitems * sizeof_item) (see gnuradio/gnuradio-runtime/lib/buffer.cc lines 76 and 121)

It seems that the criteria for allocated buffer size could instead be: an integer multiple of granularity >= (nitems * sizeof_item)

The issue this would currently cause has to do with how the circular buffers are indexed (see gnuradio/gnuradio-runtime/include/gnuradio/buffer.h starting at line 150): d_bufsize, d_write_index, d_read_index are all handled as counters of items (of size d_sizeof_item)

I believe these indices could be tracked/handled as numbers of bytes instead of numbers of items.

As best I can tell all of the changes would be made in buffer.cc and buffer.h.
Not being familiar enough with the other scheduler/buffer code files, I cannot yet tell if these are the only files that will be affected.
Can someone with more exposure to this specific code please comment? If these are the only files requiring mods, I might attempt to make the modification myself.

My specific problem involved a vector of 102400 samples of complex data.
My memory granularity size is 65536 bytes. I tried to change my vector size to 102404 samples, and I started getting the following error:

gr::buffer::allocate_buffer: warning: tried to allocate
4 items of size 819232.  Due to alignment requirements
2048 were allocated.  If this isn't OK, consider padding 
your structure to a power-of-two bytes.  
On this platform, our allocation granularity is 65536 bytes.
gr::vmcircbuf_mmap_createfilenamming: VirtualAlloc: Error 87: The parameter is incorrect
gr::buffer::allocate_buffer: failed to allocate buffer of size 1638464 bytes.

So this is saying that I NEED ~3.13Mbytes (but that's not an even integer multiple of granularity size).
GR is helping me out by trying to allocate ~1600Mbytes. With the recommended change, this need would drop to ~3.19Mbytes (that's 51 * granularity size).

This is not a new issue: https://github.com/gnuradio/gnuradio/issues/1541 https://www.ruby-forum.com/topic/212300

NotTheMattThatYouKnow avatar May 08 '18 17:05 NotTheMattThatYouKnow

If a change like this was made, changes to buffer.h and buffer.cc should be sufficient, since all the details of buffer indexing are private to the buffer class. 50 would be the right answer here, since the 51st page is really there because the original calculation assumed item sizes < page size and the +1 page would be for wrapping.

In your example, things work out nicely, but even after the change that fixes your case, it would still be possible to specify item sizes that will result in huge buffers. We don't want to put too many limitations on users, but would using 1024*128 instead of 1024*100 make more sense in the first place? Especially if this has to do with a FFT.

willcode avatar May 09 '18 10:05 willcode

If we ignored the indexing implications for now, added a class variable to hold buffer size in bytes (d_bufsizeinbytes), and changed buffer.cc: From:

	121     if(nitems % min_nitems != 0) 
	122       nitems = ((nitems / min_nitems) + 1) * min_nitems;
	...
	136     d_bufsize = nitems;
	137     d_vmcircbuf = gr::vmcircbuf_systemconfig::make(d_bufsize * d_sizeof_item );

To:

	121     d_bufsizeinbytes = nitems * sizeof_item;
	122     if(d_bufsizeinbytes % granularity != 0) 
	123     	d_bufsizeinbytes = (d_bufsizeinbytes/granularity + 1) * granularity; //rounds to next higher granularity step
	...
	137     d_vmcircbuf = gr::vmcircbuf_systemconfig::make(d_bufsizeinbytes);

This would only allow the memory allocated to grow from the absolute minimum required to the next page boundary. Since the minimum allocation size is 1 memory page, this shouldn't affect small buffer sizes.

The part that bothers me is the impact from: 78 return page_size / boost::math::gcd (type_size, page_size); This constraint is only present due to the way the buffer is indexed. And this constraint can cause the inflated buffer size issue.

NotTheMattThatYouKnow avatar May 09 '18 16:05 NotTheMattThatYouKnow

even after the change that fixes your case, it would still be possible to specify item sizes that will result in huge buffers

@willcode : Can you provide a simple example with the revised code where a reasonably-sized buffer will balloon to ridiculous sizes?

NotTheMattThatYouKnow avatar May 09 '18 17:05 NotTheMattThatYouKnow

@D-to-the-Romey : I see what you mean, now. That should work. I'm wondering why it wasn't done that way originally. ~~Maybe something to do with cache alignment?~~ (no probably not)

willcode avatar May 09 '18 21:05 willcode

@D-to-the-Romey are you planning to work on a change for this?

willcode avatar May 12 '18 12:05 willcode

I had intended to try to make changes this week, but I have a family emergency, so I won't be able to look at it until at least next week. If you or someone else would like to get started instead, have at it. If nobody else starts the changes, I will try to get started next week.

My only experience with GitHub is reviewing code and downloading projects, so I haven't ever made any pull requests. But I'd love to learn how to do it.

NotTheMattThatYouKnow avatar May 14 '18 14:05 NotTheMattThatYouKnow

@D-to-the-Romey it can wait for you - it's been this way for a long time.

willcode avatar May 15 '18 10:05 willcode

Solved per #6348

marcusmueller avatar Dec 21 '23 01:12 marcusmueller