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 "all" built-in #407

Merged
merged 13 commits into from Jul 27, 2019

Conversation

@bksahu
Copy link
Member

commented May 26, 2019

Relevant Issue: #232

Todo

  • Run all world test without the optimization (done already)
  • Run all world test with only the C code replaced, basically using your C code only, and see what this finds. This is the point in time, where this has the best exposure. The node would return self, None, None and be good, i.e. definitely not do anything wrong.
  • Run all world test with the generic implementation (the one you got right now in your PR), and see what it does (any it finds bugs).
  • Look at cases to specialize and do that, again run all world test with that.
@kayhayen

This comment has been minimized.

Copy link
Member

commented May 27, 2019

As a next step, I want you to think about these:

any([0]*10000)
all([0]*10000)
sum([0]*10000)

What's notable about those, what will be need from the sequence repeat (that doesn't yet exist in that form, but will). Is there anything, that needs to be done 10000 times? Or is there a way to shortcut, and who should have that responsibility, and where to put it. For any and all alone, there is one solution in terms of optimization that could be employed, but doesn't fit sum, or at least so I am thinking. Maybe it does too.

@bksahu

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

I'm thinking of specialising that generic algorithm by adding it to ConstantRefNodes. But that won't be helpful in case of sum. However for sum we can do one thing that is to do same computation that we will do in case of any or all but return 0 in place of False. We need to pass modes (eg. any, all or sum) as arguments to our method.

@kayhayen

This comment has been minimized.

Copy link
Member

commented May 28, 2019

I was thinking (but don't change things right now that way!), we might (later) move more of these kinds of iteration centric algorithms in a base class method of the iteration handles. Like you would do more of the any computation with default implementation there. And another one for all and sum.

And then, you overload to all implement shortcuts, e.g. all(range(1,100000)) doesn't really need to use the generic algorithm that looks at elements, you just watch out for 0 to be in a range or not (or xrange for that matter). With all and ranges, stopping to check after 256 elements, can quickly become a reason, you do not get possible optimization.

And for * as a sequence multiplication, you would special handle 0 anyway, so ignoring that for now, we should make that node, one it exists (and I think it will), make that optimize to an empty sequence of the type.

But otherwise, say we only know it's non-zero, we might be able to optimize this for as long as we know that the truth check doesn't have a side effect: any(some_list * 1000) -> any(some_list) and sum(some_list * 1000) -> sum(some_list) * 1000` so those iteration handles, for some things, they can be more clever.

Then we have a spot, where the node or its iteration handle (they are friends anyway), do this, and it's much like what you were doing as computeExpressionAny, but not in the node, but in the iteration handle. When we moved the whole algorithm there, we can make it so, that iteration handles can plug and overload them.

If we pass this as a mode, or a separate method name, I think is a matter of how common you think the code is going to be. For all and any, things are going to be relatively similar, but not that easily similar without a lot of tests over a mode. Also I think it's not a lot of code you are repeating.

Also consider, @bksahu the any algorithm is as it stands, already relatively hard to grasp. Took us both a while to get where it is now. Making a function that does either any or all depending on a mode is not making it easier. And sum might make it catastrophic. So I am arguing here for those methods to be the not same, but different names, and different algorithm, even if any and all tend to have commonality, and sum also does looping, I think it doesn't justify that.

@kayhayen

This comment has been minimized.

Copy link
Member

commented May 28, 2019

But to me, and that is why I got convinced by your slot idea ultimately, the node for * and + do know how they can help to optimize sum, and should be tasked with it. And their changes are often algorithmic.

So e.g. sum(a+b) -> sum(a) + sum(b) is normally a good optimization for as long as a+b was understood well enough by the + node.

I think we have a lot of signs that we found a very good design there, @bksahu and I suggest we focus on finishing the iteration handle work in a V1.

Then we review what it is, and identify weakness, and things that would have made it easier, apply that, and then continue with all and a refined code. I have very good experience with that kind of approach.

Right now, I think, the iteration handles have one problem, that I also do not want us to deal with immediately. There is sequential iteration and random access, and we shoved it into one thing. Some of your constructors are suffering from increased complexity. And we get the problem of expression that we can do only one of those, but not the other.

I guess, we should have SequentialIterationHandleBase and RandomAccessIterationBase, and then e.g. a helper that turns one into the other. But I think we should discuss this after we finish the any work.

@kayhayen kayhayen self-requested a review May 29, 2019

@kayhayen kayhayen changed the title Optimize "all" built-ins Optimize the "all" built-in May 29, 2019

@bksahu

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2019

So, I have added the rest of the code along with the algorithm for all built-in. However, for now, I have disabled it until any node PR is merged as IterationHandles class is in there.

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

@bksahu bksahu force-pushed the bksahu:optimize_builtin_all branch from cd2f062 to 71594ca Jun 25, 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_builtin_all branch from ed00172 to 28d5941 Jul 13, 2019

@bksahu bksahu force-pushed the bksahu:optimize_builtin_all branch from 28d5941 to b491932 Jul 20, 2019

@kayhayen
Copy link
Member

left a comment

Just minor tweaks and a little bit of research, bytes, what else is always true?

truth_value = iteration_handle.getNextValueTruth()
if truth_value is StopIteration:
break
if count > 256:

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jul 20, 2019

Member

This is why I think we need to move the algorithm of all and any into the iteration base class, so we could overload it for some cases in a better way. Asking range(1,100000) and checking its first 256 values has not a lot of point, the algorithm should become hookable. Lets think about how in a joint session. Another case is [1] * 1000000 where we would be precisely possible to reduce the sequence repeat to only checking the repeated sequence.

For any the issue was less pressing, because e.g. ranges are True after 2 elements checked max, 0, 1, -> any aborts worst case.

print(all(range(20)))
print(all([0] * 20000))
print(all([0] * 255))

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jul 20, 2019

Member

As for tests, and I interest you for extending the ones in tests/optimization where we put expressions that we expect to become optimized at compile time. These tests of yours do not cover the C usage. So maybe move those to there, and take inputs from variables here, ones that Nuitka will not optimize.

This comment has been minimized.

Copy link
@bksahu

bksahu Jul 24, 2019

Author Member

I have moved both any and all tests to tests/optimization/CommonOptimizations.py. I am planning to write tests there in the future optimizations.

value_node=value,
)

if shape is ShapeTypeStr:

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jul 20, 2019

Member

What about bytes, other types, consider the typical builtin shapes.

old_node=value,
),
"new_constant",
"Predicted truth value of built-in all string type argument",

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jul 20, 2019

Member

Always quote things, here e.g. 'all', so it's clear you don't mean "all arguments" or something like that.

return (
result,
"new_constant",
"Predicted truth value of built-in all argument",

This comment has been minimized.

Copy link
@kayhayen

kayhayen Jul 20, 2019

Member

See above.

nuitka/nodes/IterationHandles.py Outdated Show resolved Hide resolved
tests/basics/BuiltinsTest.py Outdated Show resolved Hide resolved
tests/basics/BuiltinsTest.py Outdated Show resolved Hide resolved

@bksahu bksahu force-pushed the bksahu:optimize_builtin_all branch from db68d65 to 3ec0160 Jul 27, 2019

@kayhayen
Copy link
Member

left a comment

Good job, common tests could be improved and used for more things.

@kayhayen kayhayen merged commit 5eae90f into Nuitka:develop Jul 27, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

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

@kayhayen kayhayen added the gsoc2019 label Jul 27, 2019

@kayhayen kayhayen added this to the 0.6.6 milestone 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.