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

Support oneTBB 2021 #1234

Closed
wants to merge 2 commits into from

Conversation

Logarithmus
Copy link

@Logarithmus Logarithmus commented Jul 28, 2021

@jilliene

Closes #1211

I'm not sure my solution is right, I just did what oneTBB's migration guide said.

@Logarithmus Logarithmus changed the base branch from release to dev July 28, 2021 14:28
@davidgyu
Copy link
Member

Hi! Would you please submit a signed Contributor License Agreement as described here: Contributing to OpenSubdiv Thank you!

@Logarithmus
Copy link
Author

@davidgyu would signing the PDF file with PGP and emailing it to you be sufficient?

@jilliene
Copy link

Filed as internal issue #OSD-359

@davidgyu
Copy link
Member

A regular signature on the PDF emailed back to us would be best for our process. Thanks!

@Logarithmus
Copy link
Author

Logarithmus commented Aug 9, 2021

@davidgyu I've just found a bit of free time and sent a signed PDF to you ([email protected]). It's signed both by hand & with GPG.

@davidgyu
Copy link
Member

davidgyu commented Aug 9, 2021

Thank you @Logarithmus !

Comment on lines +220 to +223
tbb::global_control tbb_global_control(
tbb::global_control::max_allowed_parallelism,
numThreads
);

Choose a reason for hiding this comment

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

Changes to TBB behavior made by tbb::global_control only active wile the object is alive. So having this object as stack variable makes the whole function TbbEvaluator::SetNumThreads to has no effects.

From the global_control::~global_control specification :

Destructs a control variable object and ends it’s impact.

Copy link

@e4lam e4lam Mar 12, 2022

Choose a reason for hiding this comment

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

+1

Isn't the old code here also effectively a no-op too here? This entire SetNumThreads() method and references to it should be removed. A library like this has no business trying to control the number of threads used process wide. It should be a decision left to the library user instead.

@MoisMoshev
Copy link

Hey, I'm trying to build with your branch, but still unable to compile. Getting errors like these from msbuild:

"e:\bottleship\code\3p\OpenSubdiv\build\examples\dxViewer\dxViewer.vcxproj" (Rebuild target) (26) ->
(Link target) -> 
  LINK : fatal error LNK1104: cannot open file 'tbb12.lib'

and also

  E:\bottleship\code\3p\OpenSubdiv\examples\common\glPtexMipmapTexture.cpp(25,10): fatal error C1083: Cannot open include file: 'glLoader.h': No such file or directory

I'm trying with the following cmake options:

     -DTBB_INCLUDE_DIR='E:/bottleship/code/3p/oneapi-tbb-2021.5.0/include'
     -DTBB_tbb_LIBRARY='e:/bottleship/code/3p/oneapi-tbb-2021.5.0/lib/intel64/vc14/tbb.lib'
     -DTBB_tbb_debug_LIBRARY='e:/bottleship/code/3p/oneapi-tbb-2021.5.0/lib/intel64/vc14/tbb_debug.lib'

The libraries are there, but it's looking for tbb12, which I probably need to add to another path?
Just wondering if my setup is the issue.

@svenstaro
Copy link

So what about merging this? Sadly this missed 3.5.0 but I'd appreciate it if this went in somewhat soon.

@davidgyu
Copy link
Member

Thanks for your patience and apologies that this didn't make it into 3.5.0.
We realize this PR has been up for a long time. We'll add a note here when we know when to expect a release with this fix.

@e4lam
Copy link

e4lam commented Sep 30, 2022

I guess you could merge it but really, you might just as well delete the contents of the SetNumThreads() method as it's moving from a method that did nothing to a method that does nothing (as was commented >6 months ago).

@davidgyu
Copy link
Member

Yes, I agree with your earlier comment regarding SetNumThreads()

@RBKreckel
Copy link

It would be extremely nice if this could be merged just to keep this package and depending packages building. In any case, it seems to me like the discussion about whether the function can be further simplified could be deferred until later.

@brad0
Copy link

brad0 commented Jun 3, 2023

As a OpenBSD package maintainer I came across this. It would be nice to have a proper fix upstream for TBB 2021 support.

@davidgyu
Copy link
Member

As noted above, we'd like to address the presence of SetNumThreads() in the API along with the other low level fixes.
We have this on deck for our next release (3.6.x) where we'll be able to address API changes (the current 3.5.1 release is for minor bug fixes only). Thanks again for your patience.

@davidgyu
Copy link
Member

Fixed by #1317 and #1319

@davidgyu davidgyu closed this Sep 14, 2023
@e4lam
Copy link

e4lam commented Sep 16, 2023

Thank you for the update!

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.

Adding support for oneTBB
9 participants