Thread safety by 9prady9 · Pull Request #1706 · arrayfire/arrayfire

9prady9 · 2017-01-10T07:45:04Z

Remove boost::shared_mutex usage in favor of thread local storage for cache management objects.
~~Implement thread_local alternative for OSX Platform~~ Making XCode 8.0 the minimum requirement, which has thread_local support.
Change kernel cache management to be thread local storage.
Change fft plan caches to use per thread local storage.
Maintain per thread active device
Unit tests

Access to InteropManager has to go through device manager now.

Access to fft plan cacher singleton has to go through device manager now.

pavanky

I think having file names as fooManager is a bit weird even if their class names are that way (even weirder when their class names are different).

I suggest simplyfing the naming mechanism and keeping the file names simple. For example:

memoryManager.* should just be part of memory.*.
interopManager.* should just be graphics.*
GfxInteropManager should just be GraphicsManager
cublasManager.* should just be cublas.*
same with cufft, cusolverDn, cusolverSn, clfft, clblas.

pavanky · 2017-01-10T07:55:23Z

-        mVendorId = "Unkown, probably ARM";
-        cout<< "Unexpected vendor id" <<endl;
+        mVendorId = "Unkown";
+        std::cout<< "Unexpected vendor id" << std::endl;


Does this need to be printed out ?

A left over i think, i will remove it.

pavanky · 2017-01-10T07:56:15Z

 bool isDoubleSupported(int device)
 {
-    return true;
+    return DeviceManager::IS_DOUBLE_SUPPORTED;


Why change from true?

Just moved all(possible) hard coded constants into single location(static data members of DeviceManager class).

pavanky · 2017-01-10T07:58:42Z

+        flag = true;
+    }
+    return 0;
+}


We should probably consider returning number of devices == number of threads.

pavanky · 2017-01-10T08:01:36Z

+
+        friend queue& getQueue(int device);
+
+        CPUInfo getCPUInfo() const;


Where else (outside of platform.cpp) is getCPUInfo going to be used?

Probably nowhere else. But i can't say we wont in future, may be while checking SSE support availability in hardware when specialising CPU kernels for respective hardware.

pavanky · 2017-01-10T08:02:06Z


+IF("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
+    SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_MWAITXINTRIN_H_INCLUDED -D_FORCE_INLINES")
+ENDIF()


Can you check if this is required for gcc 4.x ?

pavanky · 2017-01-10T08:09:18Z

+    : handle(0)
+{
+    CUSPARSE_CHECK(cusparseCreate(&handle));
+    CUSPARSE_CHECK(cusparseSetStream(handle, cuda::getStream(cuda::getActiveDeviceId())));


May be have a function called getActiveStream() so we don't have to repeatedly do this.

Makes sense

pavanky · 2017-01-10T08:12:24Z

+    }
+}
+
+}


Why is this split from memory.cpp ?

In order to use the memory manager classes for variable declaration in DeviceManager class, i moved the class declaration into hpp file. Also, I thought it would be consistent with other managers being in their own source files already - *Managers.[hpp|cpp].

pavanky · 2017-01-10T08:12:31Z

+    }
+};
+
+}


Why is this split from memory.hpp?

pavanky · 2017-01-10T08:15:21Z

+{
+    cusparseHandles[device].reset(new cusparse::cusparseHandle());
+}
+


Why unique pointers? There's no reason for any of them to be unique pointers.

I will go through them again. The sections of code that was managing sparse, solve and blas handles was using boost::scoped_ptr earlier, which i replaced with it's std equivalent unique_ptr.

pavanky · 2017-01-10T08:17:17Z

+
+        std::unique_ptr<cusolver::cusolverDnHandle> cusolverHandles[MAX_DEVICES];
+
+        std::unique_ptr<cusparse::cusparseHandle> cusparseHandles[MAX_DEVICES];


Why are some unique_ptr while others are not?

In some cases, the respective resource manager class itself uses some information that is initialised during the creation of DeviceManager class which is causing infinite loop. Therefore, for such cases, i am lazily initialising them using unique_ptrs.

Forgot to remove these files in earlier commit

Minimum required CUDA version currently is 7.0, hence the checks are not needed any more.

This abbrievates the lengthy fn call getStream(getActiveDeviceId())

Add a memory manager stress test.

Also, fixes memory leaks in medfilt and morph unit tests

Disable memory management stress test for CPU backend.

umar456 · 2017-04-30T01:03:17Z

build arrayfire windows ci

umar456 · 2017-04-30T02:03:08Z

@9prady9 ~~Looks like there are a couple of failures on Windows. Can you check it out?~~

Edit: Fixed it

pavanky · 2017-05-01T12:59:04Z

build arrayfire windows ci

pavanky · 2017-05-01T14:18:36Z

@umar456 @9prady9 windows is still broken.

9prady9 · 2017-05-01T14:20:15Z

@pavanky I believe canny's narrowing error is still present in this branch.

Edit: I will look at the other errors.

pavanky · 2017-05-01T17:11:19Z

@9prady9 can you merge from devel?

9prady9 · 2017-05-01T17:12:10Z

@pavanky Will do and push along with other Windows fixes.
Edit: @umar456 is taking care of it apparently.

umar456 · 2017-05-01T17:22:27Z

build arrayfire windows ci

umar456 · 2017-05-07T00:34:32Z

build arrayfire linux ci
build arrayfire osx ci
build arrayfire windows ci

umar456 · 2017-05-17T18:36:42Z

build arrayfire windows ci

9prady9 added 18 commits December 20, 2016 14:17

Modify CPU backend device management to be thread safe

a05206c

Merge branch 'devel' into thread_safety

d54c9d4

Merge cuda::InteropManager into cuda::DeviceManager

ddf6fde

Access to InteropManager has to go through device manager now.

Merge cufft plan cacher into cuda::DeviceManager

6ff8d91

Access to fft plan cacher singleton has to go through device manager now.

Merge cublas handle manager into cuda::DeviceManager

b789dcf

Make cuda Manager objects access less hierarchical

5e4d401

Wrap interopManager pointer with boost::scoped_ptr

b754194

Merge cusolver handle manager into cuda::DeviceManager

de3f9f7

Merge cusparse handle manager into cuda::DeviceManager

5689b84

Merge cuda memory manager into cuda::DeviceManager

8de2a59

fix cuda backend sub-managers initilization order

b204faf

Added locks for memory manager initialization

2117571

Merge branch 'devel' into thread_safety

faeb8e3

Fix for CUDA usage with gcc > 5.x

15cc018

Move clfft intialization into opencl::DeviceManager

aaca5f9

Memory-managers(opencl) initialize from DeviceManager

d2cc3ba

Init opencl gfx manager from DeviceManager

a7dd2e8

Fix gfx manager initialization to be thread safe

4360a34

9prady9 added the in-progress label Jan 10, 2017

pavanky reviewed Jan 10, 2017

View reviewed changes

9prady9 added 10 commits January 11, 2017 12:18

Remove unused link libraries from link command

ab74af8

Remove unused directory from src/backend

09da66a

Abstract InteropManager(cuda/opencl) logic using CRTP

db3b747

Abstract cuda/opencl fft plan caching logic using CRTP

c05c29b

Abstract cu{sparse|blas|solve} handle RAII wrapper

be345da

Remove obsolete files

c27933d

Forgot to remove these files in earlier commit

Remove unnecessary std::cout

8387dad

Remove checks for CUDA versions less than 7.0

c7b07a0

Minimum required CUDA version currently is 7.0, hence the checks are not needed any more.

Add new getActiveStream fn in cuda backend

25b7d90

This abbrievates the lengthy fn call getStream(getActiveDeviceId())

Change memory manager classes to use CRTP

b2e54ff

9prady9 added 3 commits April 26, 2017 09:58

Merge pull request #3 from umar456/thread_safety

7f36ff8

Add a memory manager stress test.

Replace new/deleta with std::vector in image processing tests

48482d4

Also, fixes memory leaks in medfilt and morph unit tests

Fix memory scope tests in threading unit tests

657c1ea

Disable memory management stress test for CPU backend.

9prady9 force-pushed the thread_safety branch from 85577cc to 657c1ea Compare April 26, 2017 17:46

umar456 added 2 commits April 29, 2017 17:37

Make sparse_common function static. c++11 flag for single file test

9154174

Disable memory stress test

b86e5df

umar456 added 2 commits April 29, 2017 22:59

Added missing headers

5f53f6b

Fix narrowing error/warning in canny

9ba6a96

9prady9 changed the title ~~[WIP]: Thread safety~~ Thread safety May 1, 2017

Merge branch 'devel' into thread_safety

ab7e684

Fix headers in the CUDA backend

77edd69

Remove static storage qualifier from thread_local variables

913e1ab

Merge branch 'devel' into thread_safety

d94b703

umar456 added ready to merge and removed in-progress labels May 17, 2017

Move solve threading tests to solve_dense.cpp

d290765

umar456 merged commit 2ff37fc into arrayfire:devel May 17, 2017

mlloreda added this to the v3.5.0 milestone May 22, 2017

9prady9 deleted the thread_safety branch June 23, 2017 20:24

matkraj mentioned this pull request Jan 23, 2018

Architecture for high throughput LiberTEM/LiberTEM#10

Closed


		friend queue& getQueue(int device);

		CPUInfo getCPUInfo() const;

+                  }
+              }
+              }

+                  }
+              };
+              }


		std::unique_ptr<cusolver::cusolverDnHandle> cusolverHandles[MAX_DEVICES];

		std::unique_ptr<cusparse::cusparseHandle> cusparseHandles[MAX_DEVICES];

Conversation

9prady9 commented Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavanky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

9prady9 Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umar456 commented Apr 30, 2017

Uh oh!

umar456 commented Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavanky commented May 1, 2017

Uh oh!

pavanky commented May 1, 2017

Uh oh!

9prady9 commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavanky commented May 1, 2017

Uh oh!

9prady9 commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

umar456 commented May 1, 2017

Uh oh!

umar456 commented May 7, 2017

Uh oh!

umar456 commented May 17, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

9prady9 commented Jan 10, 2017 •

edited

Loading

9prady9 Jan 10, 2017 •

edited

Loading

umar456 commented Apr 30, 2017 •

edited

Loading

9prady9 commented May 1, 2017 •

edited

Loading

9prady9 commented May 1, 2017 •

edited

Loading