Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some minor fixing which found in numpy tests #1064

Merged
merged 3 commits into from Apr 18, 2016

Conversation

Daetalus
Copy link
Contributor

  • Add __format__ function to long object. Use generic format function(object_format) will have many redundant calls and can not work with d format encode.

Mark as "WIP", because it may contains other minor NumPy fixing soon.

@undingen
Copy link
Contributor

Looks like this is mixing CAPI exceptions with C++ ones. (e,g. if PyObject_Str returns NULL this will just return NULL) - either switch everything to out C++ mechanism or just register the function as CAPI one.
And could you please check if we need this int too? (and if required could you please add it. thanks)

@kmod kmod added the wip label Feb 13, 2016
@Daetalus Daetalus changed the title [WIP] Some minor fixing. Some minor fixing which found in numpy tests Feb 16, 2016
@Daetalus
Copy link
Contributor Author

@undingen updated, would you mind to review it if you have time, please?

@undingen
Copy link
Contributor

mmh I'm finding this exact type checks very strange.
cpyhon & pypy:

>>> class I(int): pass
>>> (6).__div__(I(2))
3

I think the difference is that cpython checks if one of the the operands class is a subclass of the other one and than priorities that one over the other one which our binop does not do.

inside binary_op1:
[...]
    if (slotv) {
        if (slotw && PyType_IsSubtype(w->cls, v->cls)) {
            x = slotw(v, w);
            if (x != Py_NotImplemented)
                return x;
            Py_DECREF(x); /* can't do it */
            slotw = NULL;
        }
        x = slotv(v, w);
        if (x != Py_NotImplemented)
            return x;
        Py_DECREF(x); /* can't do it */
    }
    if (slotw) {
        x = slotw(v, w);
        if (x != Py_NotImplemented)
            return x;
        Py_DECREF(x); /* can't do it */
    }
[...]

I haven't confirmed it but from the code it looks like this is the cause.

@undingen
Copy link
Contributor

ok I'm pretty sure now that that is the issue :-(.
Could you try to to change the binop implementation instead please when you have time.
Thanks for working on this!
I think this difference is quite important and I'm glad you discovered this :-).

@kmod
Copy link
Collaborator

kmod commented Mar 19, 2016

Looking through some old PRs -- does it make sense to try to merge parts of this in? I think the binop stuff might be moot now with Marius's changes but it looks like the long.__format__ would still be nice if it didn't already make it's way in.

@Daetalus
Copy link
Contributor Author

@kmod Thanks for your comment! I am working on a patch, try to handle AST::Mult and AST::Add individually. Now the code could work correctly except the stackless error. I am thinking about your reply in Gitter. I'm agree with your comment and will split the patch to another PR. Thanks again!

@@ -868,27 +868,15 @@ template <ExceptionStyle S> static BoxedFloat* _floatNew(Box* a) noexcept(S == C

return res;
} else {
static BoxedString* float_str = internStringImmortal("__float__");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe just replace this whole __floatNew function implementation with a call to:

if (PyString_CheckExact(x))
        return PyFloat_FromString(x, NULL);
    return PyNumber_Float(x);

Because I think everything we do here PyNumber_Float should already do

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and if this change was not only to remove code duplication: could you maybe add a small test case which now works but did not before

@undingen
Copy link
Contributor

I added one small comment else looks good. 👍
Thanks

@Daetalus
Copy link
Contributor Author

@undingen Thanks for review the PR. This is updated version. And I think we can't get rid of _floatNew completely.

}
return static_cast<BoxedFloat*>(r);
}
static Box* _floatNew(Box* x) noexcept {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry forgot to mention that this still needs to handle the different exception styles:
so maybe something like this will do:

template <ExceptionStyle S> static BoxedFloat* _floatNew(Box* a) noexcept(S == CAPI) {
    Box* rtn;
    if (PyString_CheckExact(x))
        rtn = PyFloat_FromString(x, NULL);
    else
        rtn = PyNumber_Float(x);
    if (!rtn && S == CXX)
        throwCAPIException();
    return rtn;
}

@undingen
Copy link
Contributor

Sorry for another comment - I should have seen this before :-(:
Please remove the __floordiv__ float implementation too.
And we also need to remove the intFloordivFloat, intDivFloat and intTruedivFloat functions, because they are still registered and callable while they should not be:

>> (1).__floordiv__(1.0)
1.0
>> (1).__truediv__(1.0)
1.0
>> (1).__div__(1.0)
1.0

@Daetalus
Copy link
Contributor Author

@undingen , I get your point. Just FYI, once I try to remove these duplicated implementation, and got huge performance impact. I am not sure I discussed with you or @kmod . Seems JIT need those specific typed implementation. There also has other duplicated implementation, such as int.__add__(float), int.__mul__(float), int.__sub__(float), int.__pow__(float).

@Daetalus
Copy link
Contributor Author

Here is the perf impact after removing the intXXXFloat things in int.cpp. Put it here for reference.

Comparing to '['baseline']'
              pyston (calibration)                      :   0.65s baseline: 0.65 (  0.5%)
              pyston django_template3_10x.py            :   9.53s baseline: 9.55 ( -0.2%)
              pyston pyxl_bench_10x.py                  :   8.33s baseline: 8.33 ( -0.1%)
              pyston sqlalchemy_imperative2_10x.py      :  11.06s baseline: 10.93 (  1.2%)
              pyston django_template3.py                :   1.64s baseline: 1.69 ( -2.9%)
              pyston pyxl_bench.py                      :   1.10s baseline: 1.10 (  0.6%)
              pyston pyxl_bench2.py                     :   0.69s baseline: 0.70 ( -0.9%)
              pyston sqlalchemy_imperative2.py          :   1.51s baseline: 1.52 ( -1.0%)
              pyston pyxl_bench2_10x.py                 :   5.99s baseline: 5.93 (  1.0%)
              pyston (geomean-8ce1)                     :   9.57s baseline: 9.55 (  0.3%)

@Daetalus
Copy link
Contributor Author

@undingen updated! Thanks for your help!

@undingen undingen merged commit 3ec9cf9 into pyston:master Apr 18, 2016
@undingen
Copy link
Contributor

thanks for implementing the PyFloat_CheckExact approach :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants