Skip to content

Local memory pool experiment#16421

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
mshabunin:add-local-pool
Feb 7, 2020
Merged

Local memory pool experiment#16421
opencv-pushbot merged 1 commit intoopencv:3.4from
mshabunin:add-local-pool

Conversation

@mshabunin
Copy link
Copy Markdown
Contributor

@mshabunin mshabunin commented Jan 24, 2020

This pullrequest changes

Introduces cv::util::BufferArea class automating creation of structured memory areas for local memory buffers.

There are two modes: safe and unsafe. In safe mode each allocation is performed immediately, in unsafe mode (default) single large buffer will be allocated during the call to commit and all pointers will be set to appropriate locations inside this buffer. Safe mode can be forced at runtime by constructor parameter for one specific object or using an environment variable for all objects (OPENCV_BUFFER_AREA_ALWAYS_SAFE). Safe mode will also be forced for sanitizer-enabled builds.

Single buffer split into several smaller ones is used in several algorithms to reduce memory allocations for better performance. However this approach does not allow to catch OOB access using valgrind or address sanitizer and is prone to errors in address arithmetic calculations. Proposed class allows fast but unsafe mode by default and slower but safer mode for instrumentation and debugging.

Example of use:

int * buf1 = 0;
double * buf2 = 0;
cv::util::BufferArea area;
area.allocate(buf1, 200); // buf1 = new int[200];
area.allocate(buf2, 1000, 64); // buf2 = new double[1000]; - aligned by 64
area.commit();
// NOTE: area stores references to buf1 and buf2 and effectively owns them

Example of old code doing the same work :

char * buf = malloc(200 * sizeof(int) + 1000 * sizeof(double) + some_alignment_reserve);
int * buf1 = (int*)alignPtr(buf, sizeof(int));
double * buf2 = (double*)alignPtr((char*)buf1 + 200 * sizeof(int), 64);
//...
free(buf);

This pull request also proposes refactoring of StereoBM algorithm using introduced class. Note: buffers aligning to 64 bytes has been disabled during refactoring.

TODO list:

  • Test
  • Documentation
  • Use in real algorithm
buildworker:Custom=linux-1
build_image:Custom=centos:7

@mshabunin mshabunin marked this pull request as ready for review January 27, 2020 15:17
@dzhura
Copy link
Copy Markdown

dzhura commented Jan 28, 2020

stereoBM throws segfault when the input images are of size approx. 6000 by 9000 pixels. It is caused by overflow of bufSize at the line 1211

Does this patch solves the issue?

@mshabunin
Copy link
Copy Markdown
Contributor Author

@dzhura , I think it can solve the issue, especially if safe mode will be used for local buffer. Though we will not know until we try it. Do you have minimal reproducer app?

@dzhura
Copy link
Copy Markdown

dzhura commented Jan 29, 2020

@mshabunin

#include <opencv2/calib3d.hpp>

int main()
{
	cv::Mat left(8500, 6000, CV_8UC1, cv::Scalar(0));
	cv::Mat right(8500, 6000, CV_8UC1, cv::Scalar(0));

	cv::Ptr<cv::StereoBM> matcher = cv::StereoBM::create(256, 31);

	cv::Mat disparities;
	matcher->compute(left, right, disparities); // kaboom

	return 0;
}

There is no segfalut when the number of rows is 8000 or less

@mshabunin
Copy link
Copy Markdown
Contributor Author

Your sample does not crash with this patch (on 64-bit system). Valgrind does not show errors too.

@dzhura
Copy link
Copy Markdown

dzhura commented Jan 29, 2020

Thank you! I look forward to this patch to be merged

@mshabunin mshabunin changed the base branch from master to 3.4 January 30, 2020 11:58
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LocalPool alternative names:

  • MemoryZone
  • ScopedAllocatorZone
  • LocalAllocator
  • ScopedAllocator

Need to reduce amount of "new" calls and/or vector push_back() calls.

New class BufferArea is used to hide complexity of buffers allocations and allow instrumentation with valgrind and sanitizers.
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! Thank you 👍

@opencv-pushbot opencv-pushbot merged commit 55cdeaa into opencv:3.4 Feb 7, 2020
@alalek alalek mentioned this pull request Feb 10, 2020
@mshabunin mshabunin deleted the add-local-pool branch July 14, 2020 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants