These are chat archives for symengine/symengine

9th
Jul 2015
Francesco Biscani
@bluescarni
Jul 09 2015 12:38
@Sumith1896 are you there?
or @isuruf as well
I think I have a fix for the performance regression
Just gonna dump it here so maybe you can test if this solves the problem for you as well
In <piranha/thread_pool.hpp>
replace class thread_pool: private detail::thread_pool_base<> with template <typename = void> class thread_pool_: private detail::thread_pool_base<>
then, after the thread_pool_ class, around line 340, add
using thread_pool = thread_pool_<>;
so basically we rename thread_pool to thread_pool_ (notice the underscore) and then we make it a template class
and we define an alias thread_pool = thread_pool_<> in order not to break existing code
Francesco Biscani
@bluescarni
Jul 09 2015 12:43
now thread_pool is a templated class and does not get instantiated unless actually used
this seems to fix the performance regression on my side
if this works for you I will commit it to the master branch after proper documentation of the changes
and after making sure everything still works of course :)
Sumith Kulal
@Sumith1896
Jul 09 2015 12:48
Yes let me try it
Francesco Biscani
@bluescarni
Jul 09 2015 12:50
mhm maybe I talked too early... it fixes the problem with the inclusion of thread_pool but the global piranha.hpp header still seems to cause slowdown
anyway if you can check about thread_pool I can move on to locate the next cause of slowdown
Sumith Kulal
@Sumith1896
Jul 09 2015 12:51
So I can include mp_integer and hash_set without slowdown?
Francesco Biscani
@bluescarni
Jul 09 2015 12:51
try first including mp_integer and thread_pool
this used to cause slowdown right?
Sumith Kulal
@Sumith1896
Jul 09 2015 12:51
Yes, let me try
Francesco Biscani
@bluescarni
Jul 09 2015 12:52
cheers
Sumith Kulal
@Sumith1896
Jul 09 2015 13:13
@bluescarni Sumith1896/csympy@35f384a This became a good commit after amendment
Francesco Biscani
@bluescarni
Jul 09 2015 13:15
are you sill including piranha.hpp or just thread_pool + mp_integer?
Sumith Kulal
@Sumith1896
Jul 09 2015 13:16
latter
Francesco Biscani
@bluescarni
Jul 09 2015 13:16
ok nice... this is consistent with what I am seeing over here
I am re-bisecting the headers to see which one is causing troubles now
I'll keep you posted
Sumith Kulal
@Sumith1896
Jul 09 2015 13:17
Cool, I'm scraping through our old chat logs :smile:
So that we can have good documentation of all this
Francesco Biscani
@bluescarni
Jul 09 2015 13:19
ok! I have some ideas where this might come from, I hope they are true so we can keep this rational :)
Francesco Biscani
@bluescarni
Jul 09 2015 13:34
alright I might have gotten it
so what happens is that piranha.hpp includes settings.hpp, which in turn defines a non-template function called set_n_threads() which internally invokes the thread pool
so basically including settings.hpp forces the instantiation of thread_pool
the same fix adopted for thread_pool does the trick
that is, the settings class becomes template <typename = void> class settings_
Sumith Kulal
@Sumith1896
Jul 09 2015 13:37
Cool, I'm on it
Francesco Biscani
@bluescarni
Jul 09 2015 13:37
and then you add the same alias using settings = settings_<>; after the definition of the settings_ class
now it seems like including piranha.hpp does not incur in slowdowns anymore
Francesco Biscani
@bluescarni
Jul 09 2015 13:48
the only caveat is that once you call any method of the settings class, not necessarily thread_pool-related, this will force the instantiation of the set_n_threads() method, and, by extension, of the thread_pool class
something to keep in mind if you ever need to tweak piranha's settings from symengine
I suppose it's possible to make only the set_n_thread() method template... mhm I need to think about this
Sumith Kulal
@Sumith1896
Jul 09 2015 14:42
Yes, that commit comes clean with inclusion of piranha.hpp
Thanks @bluescarni
I think I should try this out in HEAD
Francesco Biscani
@bluescarni
Jul 09 2015 14:53
np! let me know how it goes, I will push a version of the fixes soon to the master
it would be nice to have some kind of performance regression testing in place for this type of problem
just in case something slips in into Piranha in the future that restores the bad behaviour
Sumith Kulal
@Sumith1896
Jul 09 2015 15:09
The references can be improved, but I have covered all the important points for the read https://github.com/sympy/symengine/wiki/En-route-to-Polynomial
ping @certik
Francesco Biscani
@bluescarni
Jul 09 2015 16:13
@Sumith1896 a minor comment: it's not clear exactly where the threading-related slowdown is coming from, but, based on what @isuruf said earlier about the memory allocation being more expensive when the slowdown arises, I suspect that it's the low-level memory allocator that is switched to a thread-safe version