Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLEvaluator doesn't separate CL program compilation and kernels #1210

Open
williamkrick opened this issue Dec 9, 2020 · 2 comments
Open

Comments

@williamkrick
Copy link

The CLEvaluator class holds and owns both a cl_program and cl_kernels. This is not ideal for performance. A cl_program represents some compiled OpenCL code that can be used to generate cl_kernels. We have found that on AMD graphics cards, and to a lesser extend on OSX, that compiling the OpenCL kernels used for OSD is quite slow. For this reason we want to compile the kernels once and re-use them.

There is a way to do this with OSD, and that is to re-use the CLEvaluator. That would be fine except we want to use the CLEvaluator in threaded code, and the cl_kernels are not thread safe! Each cl_kernel represents a function call, if we have multiple threads setting arguments into the same cl_kernel & running them then we'll have data races everywhere.

To work around this I'm using a very-slightly modified version of OSD. The modification I made was to change the CLEvaluator data members from private to protected. With that change I am able to derive from CLEvaluator and change it's behavior with respect to cl_program compilation. I added a cl_program cache which gets checked for an existing compiled version of the program before compiling it. This change is enough to resolve my performance issue.

My question is would it be possible to update the Pixar version of OSD to have protected data members rather than private data members? If so then I won't have to make any changes to OSD itself to work around the performance problem.

@jilliene
Copy link

Filed as internal issue #OSD-343

@c64kernal
Copy link
Contributor

Thanks so much for raising this @williamkrick -- there is definitely an opportunity here for a better refactoring. We'll add this to our list to consider for the next substantial release of OSD.

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

No branches or pull requests

3 participants