These are chat archives for symengine/symengine

23rd
Feb 2016
Isuru Fernando
@isuruf
Feb 23 2016 05:03
@certik, what should be the naming convention?
For eg. what should be the name of the function that wraps mpz_pow_ui?
fmpz_pow_ui is the function from flint
Ondřej Čertík
@certik
Feb 23 2016 15:21
@isuruf well that's the design question ---- why not to make them static members of the class itself? So integer_class::mpz_pow_ui for now. I think since we are taking the time to refactor integers, we should do it in a way so that we can use other libraries than mpz. Here is one MIT licensed integer library: https://github.com/creachadair/imath, here is a BSD licensed integer library: https://github.com/wbhart/bsdnt
So perhaps rather than mpz_pow_ui, we should just have pow_ui, and each class implements it differently.
Francesco Biscani
@bluescarni
Feb 23 2016 15:31
Isuru Fernando
@isuruf
Feb 23 2016 15:31
I went with mp_pow_ui for now. I'll fix that
Ondřej Čertík
@certik
Feb 23 2016 15:33
@bluescarni yes, and that too.
Isuru Fernando
@isuruf
Feb 23 2016 15:34
with the gmp wrapper class I wrote, expand2 takes 1131ms whereas gmpxx takes 974ms
I'm not really sure what the speed difference is due to
Francesco Biscani
@bluescarni
Feb 23 2016 15:34
expression templates maybe
Isuru Fernando
@isuruf
Feb 23 2016 15:36
but the computations are not done with the C++ class at all. It's only the SymEngine Integer class that wraps the C++ low level class that comes into play
Francesco Biscani
@bluescarni
Feb 23 2016 15:37
maybe I did not understand what is being compared
I thought you were using mpq_class and its overloaded operators
Ondřej Čertík
@certik
Feb 23 2016 15:39
@isuruf you have to be very careful how you wrap a class. I think in order to have zero overhead for function argument types, it needs to by copy constructible. Which is probably not possible with gmp, so maybe the overhead is coming from having this as an argument.
Ondřej Čertík
@certik
Feb 23 2016 15:40
In general, the way I see it is that the general symbolics doesn't have to be templated on the integer type, we just configure it at compile time. We can template low level data structures like the Piranha polynomials, there I think it makes sense to have a general code.
@isuruf so you are wrapping the mpz type directly. Yes, I like that a lot. Let me read it some more.
Is this correct: inline mpz_ptr get_mpz_t() { return mp; }? mp is mpz_t, but you are returning mpz_ptr.
Francesco Biscani
@bluescarni
Feb 23 2016 15:43
mpz_t is an array of size one, so it should be ok
Isuru Fernando
@isuruf
Feb 23 2016 15:43
There are 4 types in that branch
cmake -DINTEGER_CLASS=gmp . uses the gmp wrapper,
cmake -DINTEGER_CLASS=gmpxx . uses mpz_class,
cmake -DINTEGER_CLASS=flint . uses flint wrapper,
cmake -DINTEGER_CLASS=piranha . uses piranha::integer,
Ondřej Čertík
@certik
Feb 23 2016 15:44
@isuruf I think you also need to implement (or delete or default) the assignment and move operators (not just the constructors)
Ah you do, I missed that.
Francesco Biscani
@bluescarni
Feb 23 2016 15:46
In here: https://github.com/isuruf/symengine/blob/int/symengine/mp_wrapper.h#L462 you are implementing a move operator that allocates internally, right?
move constructor sorry
Ondřej Čertík
@certik
Feb 23 2016 15:47
Right.
Isuru Fernando
@isuruf
Feb 23 2016 15:47
Shouldn't other be in a valid state after move?
Ondřej Čertík
@certik
Feb 23 2016 15:47
It needs to be in a valid state, but perhaps there is a faster solution that avoids allocation/deallocation.
Francesco Biscani
@bluescarni
Feb 23 2016 15:47
that's a tradeoff that you need to decide, let me explain
usually you want to implement move semantics as fast as possible, as you need them to be efficient in a variety of scenarios
but of course you have the problem with mpz_t that always require an allocated state to be "valid"
so you can to change your definition of valid
what I do in this case is that I set the values of the mpz struct to zero
and then I add a special casing in the assignment operators and in the dtor
that checks if the mpz was "moved from"
having a class which can be assigned to and destroyed after a move operation is generally all you need to make the class usable with most C++ standard algorithm
so you pay a bit more complexity in assignment and destruction with faster moves
Ondřej Čertík
@certik
Feb 23 2016 15:50
@bluescarni if you move a class, the original class can only be destroyed, correct?
Francesco Biscani
@bluescarni
Feb 23 2016 15:51
you can decide that by yourself, but in any sane scenario it should be at least destructible
and most preferably assignable
so you can "revive" it
Ondřej Čertík
@certik
Feb 23 2016 15:51
ah, you mean assign to?
Francesco Biscani
@bluescarni
Feb 23 2016 15:51
this is all that's required, for instance, by std::sort
Ondřej Čertík
@certik
Feb 23 2016 15:52
Why would std::sort assign to a moved from class?
Francesco Biscani
@bluescarni
Feb 23 2016 15:52
yes... if I move from x, then I should be able to do x = something;
because you swap elements around during the sort
instead of copying them
but this require that you need to be able to assign to an object that was previously moved
Ondřej Čertík
@certik
Feb 23 2016 15:52
I think it's a deficiency of C++ --- the language could instead guarantee to simply "remove" the moved from class completely, and if you assign back to it, it would treat it as a new class.
Unless, of course, the class is stored somewhere else than in a local scope...
Francesco Biscani
@bluescarni
Feb 23 2016 15:53
it's a tradeoff, I think there were alternative proposals floating around regarding move semantics
Ondřej Čertík
@certik
Feb 23 2016 15:54
I think the current move semantics doesn't go far enough, so you still pay for what you don't use.
Francesco Biscani
@bluescarni
Feb 23 2016 15:54
in which way?
@isuruf the approach I described is adopted in the real class if you want to take a look: https://github.com/bluescarni/piranha/blob/master/src/real.hpp
Isuru Fernando
@isuruf
Feb 23 2016 15:56
@bluescarni, thanks, I just tried a solution and the timings are now the same
Francesco Biscani
@bluescarni
Feb 23 2016 15:56
ok cool :)
Ondřej Čertík
@certik
Feb 23 2016 15:58
@bluescarni you pay extra, because you need to do the checks if the class was moved from, before you deallocate the internal data structures in the destructor.
Isuru Fernando
@isuruf
Feb 23 2016 15:58

    inline mpz_wrapper(mpz_wrapper&& other) {
        mp->_mp_d = nullptr;
        mpz_swap(mp, other.get_mpz_t());
    }
    inline mpz_wrapper& operator=(const mpz_wrapper& other) {
        if (mp->_mp_d == nullptr) {
            mpz_init_set(mp, other.get_mpz_t());
        } else {
            mpz_set(mp, other.get_mpz_t());
        }
        return *this;
    }
    inline mpz_wrapper& operator=(mpz_wrapper&& other) {
        mpz_swap(mp, other.get_mpz_t());
        return *this;
    }
    inline ~mpz_wrapper() {
        if (mp->_mp_d != nullptr) {
            mpz_clear(mp);
        }
    }
Is this okay?
Francesco Biscani
@bluescarni
Feb 23 2016 16:00
looks good on first sight
you just have to be careful from now on to be aware of the behaviour of moved-from objects, and document it
note that in "normal" usage nothing will change
most moved-from object are immediately destroyed
you only have to be careful when you do explicit moves
a = std::move(b);
...
a + b; // <- problem here
Ondřej Čertík
@certik
Feb 23 2016 16:02
@bluescarni I think a Debug time checks can check that the class is not "moved from" when doing a+b.
Francesco Biscani
@bluescarni
Feb 23 2016 16:03
yep
I think this situation has more to do with GMP forcing allocation rather than move semantics
Isuru Fernando
@isuruf
Feb 23 2016 16:08
It seems libgmpxx doesn't do this
but still the speed is there
Ondřej Čertík
@certik
Feb 23 2016 16:09
Maybe the expression templates that libgmpxx uses do the same, effectively.
Isuru Fernando
@isuruf
Feb 23 2016 16:10
@certik, didn't think of that. Thanks
Ondřej Čertík
@certik
Feb 23 2016 16:11
Are you getting the same performance now?
Isuru Fernando
@isuruf
Feb 23 2016 16:13
Yes. this is because the algorithms used in the benchmark doesn't reuse the mpz objects. I would expect the performance to degrade in algorithms in ntheory.cpp, but I guess that's okay
Ondřej Čertík
@certik
Feb 23 2016 16:14
Why would it decrease?
Isuru Fernando
@isuruf
Feb 23 2016 16:14
because of expression templates, there'll be speed benefits
mpz_class t = a + b;
t = t - 1;
would allocate memory to new object (t-1) whereas with expression templates you can avoid that. That's what I dedue from @bluescarni's explanation of expression templates
Ondřej Čertík
@certik
Feb 23 2016 16:16
Not if we rewrite things as discussed in #478.
In this case:
mpz_class t = a + b;
t -= 1;
My understanding is that you truly do not need expression templates for performance.
It's a common misconception that expression templates are needed to avoid temporaries, but in fact, they are not. At least that is my current understanding after discussing this with @bluescarni.
Isuru Fernando
@isuruf
Feb 23 2016 16:24
Okay. I will give it a try in the weekend.
Ondřej Čertík
@certik
Feb 23 2016 16:24
Specifically, see the comment https://github.com/symengine/symengine/issues/478#issuecomment-153188107 and also https://github.com/symengine/symengine/issues/478#issuecomment-153835951, so if we do not want expression templates (and we don't), we really want to be using just the mpz_mul(d.get_mpz_t(), h.get_mpz_t(), p.get_mpz_t()) approach, not even overload operators.
But I think Piranha expects the coefficients to have overloaded operators, so I guess we can do both.
Isuru Fernando
@isuruf
Feb 23 2016 16:30
Oh. That's a big project (going through code in all of symengine). Better left off until my exams are over. I'll polish the code as of now and send a PR over the weekend.
There's still some stuff left for fmpz_wrapper. Things like +(fmpz_wrapper, uint) needs to be implemented.
Ondřej Čertík
@certik
Feb 23 2016 16:37
@isuruf there is also "Named return value optimization", so I am actually not sure if you lose performance when you use the operators like +.
So let's do this incrementally. Why not to finish your PR so that we can merge it, and then possibly improve upon it later?
Isuru Fernando
@isuruf
Feb 23 2016 16:38
yes, will do that
Rajith Vidanaarachchi
@rajithv
Feb 23 2016 16:38
Hi, I'm looking at the Ruby wrappers these days.. can you guys point me in a direction , for me to look at next?
Isuru Fernando
@isuruf
Feb 23 2016 16:42
@rajithv, you can look at implementing floating points or math functions
@certik, final runtimes are
935 ms, 981 ms, 732 ms and 896 ms
gmp, gmpxx, flint, piranha
for expand2
Rajith Vidanaarachchi
@rajithv
Feb 23 2016 16:45
@isuruf thanks.. I'll look into it.
Ondřej Čertík
@certik
Feb 23 2016 16:45
@isuruf how can gmp be faster than gmpxx?
@rajithv thanks for working on the Ruby wrappers.
Isuru Fernando
@isuruf
Feb 23 2016 16:46
Probably because gmpxx allocates memory on the moved object.
which would be destroyed after that
Ondřej Čertík
@certik
Feb 23 2016 16:47
Good job, let's wrap this up and merge.
We can further improve on it later.
If we can avoid gmpxx, we should do it, as we can then remove the cmake check and all the trouble with C++ ABI compatibility.
Isuru Fernando
@isuruf
Feb 23 2016 16:49
Yes, when -DINTEGER_CLASS=gmp is given, cmake won't look for gmpxx