Static variables in Profiling_tools package

[sloriot]

• [ ] Profiling_tools/include/CGAL/Timer_impl.h:123: static double prec = compute_precision();
• [ ] Profiling_tools/include/CGAL/Profile_counter.h:212: { static CGAL::Profile_counter tmp(Y); ++tmp; }
• [ ] Profiling_tools/include/CGAL/Profile_counter.h:214: { static CGAL::Profile_histogram_counter tmp(Y); tmp(Z); }
• [ ] Profiling_tools/include/CGAL/Profile_counter.h:216: static CGAL::Profile_branch_counter NAME(Y); ++NAME;
• [ ] Profiling_tools/include/CGAL/Profile_counter.h:220: static CGAL::Profile_branch_counter_3 NAME(Y); ++NAME;
• [ ] Profiling_tools/include/CGAL/Real_timer.h:57: static bool m_failed = false;
• [ ] Profiling_tools/include/CGAL/Real_timer.h:61: static bool m_failed;
• [ ] Profiling_tools/include/CGAL/Real_timer_impl.h:108: static double prec = compute_precision();
• [ ] Profiling_tools/include/CGAL/Profile_timer.h:72: static CGAL::Profile_timer CGAL_profile_timer_tmp(NAME); \
• [ ] Profiling_tools/include/CGAL/Timer.h:55: static bool m_failed = false;
• [ ] Profiling_tools/include/CGAL/Timer.h:59: static bool m_failed;

[afabri]

There is nothing to do for the Profile counters. They use tbb::atomic and a tbb::concurrent_hash_map. To require TBB in order to profile across several threads is acceptable.

[afabri]

@sloriot Can you please check and tick.

[sloriot]

It is not clear to me. For example I would have expected that for the macros CGAL_PROFILER, the static variables should be atomic. Then I'm not sure we need to make this pat of the library thread safe.

[afabri]

@lrineau suggested to make the counter inside the static variable atomic, so that we profile across all threads, assuming that we want the total number of, say filter failures of orientation tests, and not the number of failures per thread.

[lrineau]

It would be rather easy to have most of CGAL profilers thread-safe (profiling across all threads), but for the histogram counter. For the later one, we would need to use mutex/locks, or use a thread-safe map/dictionary, like the one provided by TBB.

[afabri]

Looking at the code of the Histogram_counter it seems that what you suggest is already implemented. @sloriot and @lrineau : Do I miss something?

[lrineau]

The code of Histogram_counter is only thread-safe when CGAL_LINKED_WITH_TBB is defined. Without TBB, we still use a normal std::map, not protected by any mutex/lock.

[afabri]

The Histogram_profiler is not documented, and the fact that it only works across several threads when TBB is available that seems good enough for the moment. Do you agree @sloriot ?

[sloriot]

@afabri it would be nice if you have time to look at what remains to be done for this issue so that we can close it.

[afabri]

I have read the above discussion and I had a look at the code. Can you please answer the following two questions: Do you agree that we want a global profiling and not a per-thread profiling (for example on the number of filter failures of orientation tests)? Do you agree that we can require TBB for the profiling of histograms (for example for profiling the degree of vertices for which we fill a vector with incident cells)? If you agree then all what remains to do is probably to work on the std::cout << "number of "<< N << std::endl in the destructors. I guess we first have to write into a string and then write out the string to the stream so that output does not get mixed up.

[sloriot]

1) yes 2) yes

[afabri]

And do you confirm that we first have to write into a string?

[gdamiand]

You can use printf.

[sloriot]

You mean a thread local string?

[afabri]

Have a look at the destructor of the Histogram_counter. There are many operator<<() and they might mix up as std::cerr is shared by all threads. I guess I should first write inside the destructor into an ostringstream, and once I have assembled everything I insert a single string into std::cerr, which I suppose is thread safe.

[lrineau]

insert a single string into std::cerr, which I suppose is thread safe.

Concurrent writings to std::cerr is thread-safe: that cannot crash. But the display can interleave the characters inserted by the threads.