These are chat archives for symengine/symengine

2nd
Jul 2015
Isuru Fernando
@isuruf
Jul 02 2015 14:22
@certik, can you take a look at sympy/symengine#467
Also the wiki page as well
Isuru Fernando
@isuruf
Jul 02 2015 14:53
A question related to #467, how should we print a floating point number and a floating point complex number?
Ondřej Čertík
@certik
Jul 02 2015 15:34
@isuruf I would just print the digits, probably just like Sage or SymPy does
Sumith Kulal
@Sumith1896
Jul 02 2015 16:27
@certik you there?
Regarding the slowdown I was taking about, it is coming from the commit Sumith1896/csympy@35f384a
as git bisect says
git bisect is an amazing tool though, never used it before :smile:
Isuru Fernando
@isuruf
Jul 02 2015 16:33
@Sumith1896, there's a new method introduced by @bluescarni recently to construct a piranha::integer from mpz_t. Can you try it?
Francesco Biscani
@bluescarni
Jul 02 2015 16:33
Sumith Kulal
@Sumith1896
Jul 02 2015 16:40
But that shouldn't affect the expand2 and expand2b, should it?
This statement auto v = a.second.get_mpz_view(); ?
Isuru Fernando
@isuruf
Jul 02 2015 16:43
No, it shouldn't affect expand2 and expand2b
Francesco Biscani
@bluescarni
Jul 02 2015 16:46
the slowdown is limited to expand2 and expand2b?
Sumith Kulal
@Sumith1896
Jul 02 2015 16:47
Yes
Francesco Biscani
@bluescarni
Jul 02 2015 16:49
and it does not include any packing/unpacking... mhmhm
Sumith Kulal
@Sumith1896
Jul 02 2015 16:50
expand2d is fast enough, I wouldn't mind if it speeds up :smile:
Francesco Biscani
@bluescarni
Jul 02 2015 16:50
the only thing I see is that umap_ull_mpz used to use mpz_class, now uses piranha::integer
can you walk me through checking the performance locally
?
I'd like to understand if there is something wrong with integer
Sumith Kulal
@Sumith1896
Jul 02 2015 16:53
Yes, just execute expand2 and expand2b once you build
Nothing is wrong with integer I think
Francesco Biscani
@bluescarni
Jul 02 2015 16:53
which branch?
Sumith Kulal
@Sumith1896
Jul 02 2015 16:53
my packint branch
Those containers are not used in expand2 and expand2b
Hence these benchmarks don't use integer
Francesco Biscani
@bluescarni
Jul 02 2015 16:56
do I need to build with WITH_PIRANHA?
Sumith Kulal
@Sumith1896
Jul 02 2015 16:57
yes , -DWITH_PIRANHA=yes -DWITH_MPFR=yes
MPFR support is corrected in master I think, not in my packint branch
Sumith Kulal
@Sumith1896
Jul 02 2015 17:04
Is the header inclusion slowing it down?
Francesco Biscani
@bluescarni
Jul 02 2015 17:10
sorry I had to go afk a while
it shouldn't
sympy/symengine@bab90bb this is the good commit
just to fix things:
and this is the first bad sympy/symengine@35f384a
?
Sumith Kulal
@Sumith1896
Jul 02 2015 17:12
Did I do something wrong
brb
Francesco Biscani
@bluescarni
Jul 02 2015 17:13
I think my internet is messing up the chat sorry about that
what I meant is this: according to the bisect, the first bad commit is sympy/symengine@35f384a (where the slowdown happens) and the previous commit, sympy/symengine@4848c2a, is good?
ok I can observe the slowdown locally between the commits
it is something like 96ms vs 85ms on my machine, on average
Francesco Biscani
@bluescarni
Jul 02 2015 17:31
I am investigating but need to go away for a while, otherwise my pregnant girlfriend will kill me :D
Sumith Kulal
@Sumith1896
Jul 02 2015 17:35
Cool :smile:
I'll look into this
Let see what @certik has to say.
Till then let me verify this.
Francesco Biscani
@bluescarni
Jul 02 2015 18:01
mhm this is rather puzzling
so the first thing I noticed is that you can make expand2b faster by playing around with the GMP interface
try replacing C[exp] += a.second*b.second;
with mpz_addmul(C[exp].get_mpz_t(),a.second.get_mpz_t(),b.second.get_mpz_t());
in poly_mul
here it goes down from 80-90ms to around 50 ms
still does not explain the slowdown when including the Piranha headers
Francesco Biscani
@bluescarni
Jul 02 2015 18:07
this is all speculation at this moment, but one things that might be happening is the following:
actually, scratch that lol
you are never even using integer in expand2b right?
I thought that maybe the memory system was reusing previously-allocated memory in the benchmark, that now does not exist any more because you switched to integer
but it does not seem to make sense looking at the benchmark
back to the drawing board
Francesco Biscani
@bluescarni
Jul 02 2015 20:01
alright so I have a lead
turns out that if you only include piranha/mp_integer.hpp instead of piranha/piranha.hpp the performance problem disappears
piranha.hpp is a global header that includes all the public headers provided by piranha
so the next logical step was to figure out which is the header that causes the performance regression
turns out it is most likely thread_pool.hpp
so what does thread_pool.hpp does?
in short, it contains all (or most of) the multithreaded machinery used in Piranha
in particular it contains a static (i.e., global) thread pool that is used in the parallel polynomial multiplication routine
I believe that this global object is created even if no threading-related functionality is ever called or required
most importantly, I think that once the compiler sees that there is multithreading involved, it will be prevented from doing certain classes of optimisation
Francesco Biscani
@bluescarni
Jul 02 2015 20:07
I have found some references on stackoverflow, even if they refer to OpenMP mostly:
Sumith Kulal
@Sumith1896
Jul 02 2015 20:07
Sorry I was off for a bit
yes that header was the only thing fishy there
oh cool, that fixes it then
Let me try it out
You had previously mention that single header can be included. :smile:
Should have done it then.
Francesco Biscani
@bluescarni
Jul 02 2015 20:10
yeah but now I would like to figure out the extent of this discovery, or at least dig it a bit more :)
the fix for now should be to include only mp_integer.hpp
Sumith Kulal
@Sumith1896
Jul 02 2015 20:10
mpz_addmul is a good option to add in expand2b in general right?
Francesco Biscani
@bluescarni
Jul 02 2015 20:10
but if you want to use piranha polys you will run into thread_pool sooner or later
yes
Sumith Kulal
@Sumith1896
Jul 02 2015 20:11
There was a mention in a blog post I think
Francesco Biscani
@bluescarni
Jul 02 2015 20:11
yeah I used that as an example here: http://bluescarni.github.io/overloading-overloaded.html
interestingly enough, when you do the addmul thing the performance penalty seems to go away
that is, you can keep on including piranha.hpp
Sumith Kulal
@Sumith1896
Jul 02 2015 20:12
We have plans of using the containers only now, but I can't say with guarantee
Ohh
Francesco Biscani
@bluescarni
Jul 02 2015 20:12
that makes me think that the behaviour is related to memory allocation
Sumith Kulal
@Sumith1896
Jul 02 2015 20:12
But even expand2 slows down
Francesco Biscani
@bluescarni
Jul 02 2015 20:12
when you do addmul you will have far fewer memory allocations
what do you mean?
Sumith Kulal
@Sumith1896
Jul 02 2015 20:13
There is another benchmark expand2
which uses SymEngine basic expression
no polynomial structure
even that had a slowdown
So there maybe multiple areas where slowdown maybe there, I think including single headers is better
Francesco Biscani
@bluescarni
Jul 02 2015 20:15
it seems like it is enough to include the header to reproduce the slowdown
you don't have to use anything from it
ok I think I confirmed it's memory allocation
Sumith Kulal
@Sumith1896
Jul 02 2015 20:15
Go on
Francesco Biscani
@bluescarni
Jul 02 2015 20:16
can you try to compile and run with WITH_TCMALLOC
Sumith Kulal
@Sumith1896
Jul 02 2015 20:17
Yes
Sumith Kulal
@Sumith1896
Jul 02 2015 20:25
I don't have gperftools installed it seems
will take few minutes
Francesco Biscani
@bluescarni
Jul 02 2015 20:31
ok
Francesco Biscani
@bluescarni
Jul 02 2015 20:38
need to go for a while, I'll be back later
Sumith Kulal
@Sumith1896
Jul 02 2015 20:42
Np
Sumith Kulal
@Sumith1896
Jul 02 2015 20:51
with -DWITH_PIRANHA=yes -DWITH_MPFR=yes -DWITH_TCMALLOC=yes the benchmarks slow down a lot
Sumith Kulal
@Sumith1896
Jul 02 2015 21:03
Okay, the problem now is this
For that commit we can do the fix of using only mp_integer
But we want to use hash_set too
which calls the header thread_pool
We need to workaround this
Sumith Kulal
@Sumith1896
Jul 02 2015 21:18
This again will affect our designs, need to think through carefully
@bluescarni @certik