RcppParallel icon indicating copy to clipboard operation
RcppParallel copied to clipboard

Feature/update tbb 2020 2

Open SteveBronder opened this issue 5 years ago • 8 comments

Howdy!

This just runs the script in tbb-update.R for TBB 2020.2 from the travis logs of #111 it looks like there was something weird with the suppress_warnings header in 2020.1 that is fixed in 2020.2

In file included from ../../src/tbb/concurrent_hash_map.cpp:17:0:
../../include/tbb/concurrent_hash_map.h:21:54:
    fatal error: internal/_warning_suppress_enable_notice.h: No such file or directory

From #111

Still need to port Windows-specific fixes. Note: this unfortunately does not fix ASAN / UBSAN warnings.

I don't have access to a windows machine (linux/chrome-os only). Is wine the way to go for testing the windows specific patches?

SteveBronder avatar Jun 08 '20 18:06 SteveBronder

This still has two warnings

../../src/tbbmalloc/proxy.cpp:343:6: warning: the program should also define ‘void operator delete(void*, std::size_t)’ [-Wsized-deallocation]
  343 | void operator delete(void* ptr) __TBB_NO_THROW {
      |      ^~~~~~~~
../../src/tbbmalloc/proxy.cpp:347:6: warning: the program should also define ‘void operator delete [](void*, std::size_t)’ [-Wsized-deallocation]
  347 | void operator delete[](void* ptr) __TBB_NO_THROW {

From cppreference tho'

(signatures) Called instead of (1-2) if a user-defined replacement is provided, except that it's unspecified whether (1-2) or (5-6) is called when deleting objects of incomplete type and arrays of non-class and trivially-destructible class types. A memory allocator can use the given size to be more efficient. The standard library implementations are identical to (1-2 [Steve: The non-size_t signatures]).

So if we want to patch those warnings they can just be defined as

void operator delete(void* ptr, size_t sz) __TBB_NO_THROW {
    InitOrigPointers();
    __TBB_malloc_safer_free(ptr, (void (*)(void*))orig_free);
}

SteveBronder avatar Jun 08 '20 19:06 SteveBronder

ping @kevinushey

SteveBronder avatar Jun 11 '20 17:06 SteveBronder

@kevinushey looking over the new version of the tbb it looks like they handle some the memset things that you had to manually patch in before. Is there anything I can help with to make this review easier?

We've been looking at github actions for rstan and those could potentially help sanity check some of the different OS/compiler versions here as well

https://github.com/stan-dev/rstan/pull/843/files

SteveBronder avatar Sep 10 '20 23:09 SteveBronder

One thing to note here is that I had a hard time configuring the files in old to compile. But you also define -DTBB_NO_LEGACY=1 in the CXXFLAGS so I think it would be safe to delete the line here in makefile.tbb

# OLD/Legacy object files for backward binary compatibility
ifeq (,$(findstring $(DEFINE_KEY)TBB_NO_LEGACY,$(CPLUS_FLAGS)))
TBB_CPLUS_OLD.OBJ = \
		concurrent_vector_v2.$(OBJ) \
		concurrent_queue_v2.$(OBJ) \
		spin_rw_mutex_v2.$(OBJ) \
		task_v2.$(OBJ)
endif

I've looked over a good bit of the changes and it seems like they have much better support for mingw / windows now

SteveBronder avatar Sep 10 '20 23:09 SteveBronder

Thanks for continuing to work on this PR, and sorry for taking so long to respond -- it's been challenging to find time! If you can get the Linux + macOS work into a good state, I'll try to find time to get the Windows bits working as well.

I'm on board with removing old/ as well.

kevinushey avatar Sep 11 '20 18:09 kevinushey

Totally no worries! Do you have a script to run all the reverse dependencies? I think if we have that I can steal some stuff from rstan to run a github actions script to run the tests and reverse dependencies on mac/windows/linux

SteveBronder avatar Sep 11 '20 22:09 SteveBronder

I don't; I usually just run reverse dependency checks with revdepcheck.

kevinushey avatar Sep 12 '20 04:09 kevinushey

FWIW, I've been building RcppParallel against the system tbb v2020 here, and I've detected no issues from any dependency.

Enchufa2 avatar Jan 28 '23 20:01 Enchufa2