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

Optimize the "abs" built-in #419

Merged
merged 14 commits into from Jul 27, 2019

Conversation

@bksahu
Copy link
Member

commented Jun 6, 2019

Relevant Issue: #232

@kayhayen
Copy link
Member

left a comment

Pretty much perfect in my eyes.

Aside from missing out on unary operator nodes, of which there really is only not right now, I have no complaints. I think the base class for those should get better, much like the one for binary ones is getting better recently, but that can be delayed totally.

CHECK_OBJECT(o)

PyNumberMethods *m = o->ob_type->tp_as_number;
if (likely(m && m->nb_absolute))

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 7, 2019

Member

Always use blocks, even for single statements. In C this avoids so many errors.

@@ -548,6 +549,7 @@ def makeGlobalContext():
"EXPRESSION_BUILTIN_REF": generateBuiltinRefCode,
"EXPRESSION_BUILTIN_EXCEPTION_REF": generateExceptionRefCode,
"EXPRESSION_BUILTIN_ANONYMOUS_REF": generateBuiltinAnonymousRefCode,
"EXPRESSION_BUILTIN_ABS_REF": generateBuiltinAbsCode,

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 7, 2019

Member

There should be no REF. That part of the name stands for reference, and makes we are having some kind of name lookup. This is not the case here. That probably means you also misnamed the operation class.

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 7, 2019

Member

Ah, I see you did not match the names in the abs node and here, if they do not, this will simply not have an effect, and compiling to C should give an error for everything not statically optimized.



class ExpressionBuiltinAbs(ExpressionBuiltinSingleArgBase):
kind = "EXPRESSION_BUILTIN_ABS"

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 7, 2019

Member

I would have expected this one in OperatorNodes. There is going to be a lot of commonality with other unary operations. |a| in math is an unary operation, just not one for which there is Python syntax, i.e. the built-in is the only way to say it. That doesn't mean it shouldn't be where the others are.

nuitka/nodes/shapes/BuiltinTypeShapes.py Show resolved Hide resolved
tests/basics/BuiltinsTest.py Show resolved Hide resolved
)

def mayRaiseException(self, exception_type):
value = self.getValue()

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 7, 2019

Member

What does abs(float("nan")) do?

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jun 7, 2019

Member

Also, if a class overloaded __abs__ then it could raise in the slot.

I think you need a function like mayRaiseExceptionAbs() in expression nodes, that is pessimistic, for all but a few things. But now that I think of it, TypeShapeFloatDerived is based on slots and may work with this approach.

This may need more though from my side. I think I have had mayRaiseExceptionBool for some time, and only recently added the slot to type shapes. These might tie together like that or be redundant, or the base class implementation ought to use the type shape, but other parts may not.

This comment has been minimized.

Copy link
@bksahu

bksahu Jun 7, 2019

Author Member

What does abs(float("nan")) do?

It returns nan. Should I do something to deal with it?

@bksahu

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

@kayhayen the pylint error in travis is unrelated to this PR

@kayhayen

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

yes, for the pylint stuff, rebase to current develop, I messed up, there is a false alarm of pylint only on travis, that sneaked past me.

@bksahu bksahu force-pushed the bksahu:optimize_abs branch from 1c623e2 to ebe969f Jun 8, 2019

@kayhayen kayhayen force-pushed the Nuitka:develop branch from 3580003 to 38893e6 Jun 13, 2019

@kayhayen kayhayen added this to Review in progress in Planning Jul 1, 2019

@kayhayen kayhayen force-pushed the Nuitka:develop branch from 113e470 to f3367de Jul 8, 2019

@bksahu bksahu force-pushed the bksahu:optimize_abs branch from ebe969f to af416d1 Jul 13, 2019

@bksahu bksahu force-pushed the bksahu:optimize_abs branch from af416d1 to 1e407a7 Jul 20, 2019

bksahu added 4 commits Jul 20, 2019
}

#if PYTHON_VERSION >= 300
return PyErr_Format(PyExc_TypeError, "bad operand type for abs(): '%.200s'", Py_TYPE(o));

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jul 20, 2019

Member

Just use '%s'.

}

#if PYTHON_VERSION >= 300
return PyErr_Format(PyExc_TypeError, "bad operand type for abs(): '%.200s'", Py_TYPE(o));

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jul 20, 2019

Member

Should be more like Py_TYPE(o)->tp_name which is a C string.

@kayhayen

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Let me know once nightshift turned green on this latest pushes.

@kayhayen kayhayen merged commit 4c8a69b into Nuitka:develop Jul 27, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Planning automation moved this from Review in progress to Done Jul 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.