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 built-in "zip" #462

Open
wants to merge 27 commits into
base: develop
from

Conversation

@bksahu
Copy link
Member

commented Jul 12, 2019

Relevant Issue: #232

This is a work in progress.

Todo

  • Run nightshift without the optimization
  • Run nightshift with only the C code replaced, basically using your C code only.
  • Run nightshift with the generic implementation.
  • Look at cases to specialize and do that, again run all world test with that.

@bksahu bksahu changed the title C code added for zip for py27 Optimize built-in "zip" Jul 13, 2019

@bksahu bksahu force-pushed the bksahu:optimize_zip branch 2 times, most recently from 025ee21 to c5c979b Jul 17, 2019

@bksahu bksahu force-pushed the bksahu:optimize_zip branch from 7c3222a to a7e24d6 Jul 27, 2019

@bksahu bksahu force-pushed the bksahu:optimize_zip branch from ee98e53 to 1b41b0b Aug 4, 2019

@kayhayen
Copy link
Member

left a comment

Almost there. :-)

# TODO: Can you tell if arguments are not iterable

# Notice: For C side, just call zip built-in
pass

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 4, 2019

Member

That is not going to work well, I think you have to return in these overloads.


@calledWithBuiltinArgumentNamesDecorator
def wrapZipBuiltin(call_args, source_ref):
# pylint: disable=unused-argument

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 4, 2019

Member

That pylint disable seems wrong they are both used.

for i in range(len(call_args))
]

# Add check for all argument if they are iterable

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 4, 2019

Member

Left over comment, makes me wonder if the TODO is solved

exception_name="TypeError",
handler_body=makeRaiseExceptionExpressionFromTemplate(
exception_type="TypeError",
template="zip argument #%s must support iteration",

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 4, 2019

Member

Why use a template with constant arguments, seems the value is known and %d would be better suited too, either way.

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 4, 2019

Member

There may be a function that uses no template, but similarily does it.

statements.append(
StatementReturn(
ExpressionTempVariableRef(
# TODO: Create that tmp_result

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 4, 2019

Member

Without solving this TODO, it will crash (trigger an assertion with --debug) at run time, because the temp variable won't be assigned.

This comment has been minimized.

Copy link
@bksahu

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 4, 2019

Member

Allocating a variable, merely creates an object, it does nothing with it. You need to assign a value to that variable for it to be assigned. I am not referring to tmp_result as a Python level variable in your re-formulation. I am referring to it as the run time temporary variable of the outline, which without a StatementAssignVariable is going to be very crash happy if referenced before or without it.

),
source_ref=source_ref,
)
for i, _ in enumerate(zip_arg_variables)

This comment has been minimized.

Copy link
@kayhayen

kayhayen Aug 4, 2019

Member

I have used range(len()) in similar cases and I prefer it.

@bksahu bksahu force-pushed the bksahu:optimize_zip branch from 019de69 to 100161a Aug 9, 2019

@bksahu

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Isn't that what enumerate is for? The use of 'i' and 'values[i]' going over the len is less readable, although of course correct.

I thought this way is more efficient than the enumerate way because enumerate is not optimized yet. I will change it.

@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

The enumerate is on the Nuitka compile time level, we don't care about performance there. You are right that there is no built-in emumerate node, and I think we should definitely strive to get at it, as it's going to be cruical for loop optimization.

@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Also, I am unclear which branch you mean, but the code where you create a constant from zip() will refuse to do so, because it is not a compile time constant. Just put it to the test and see how your 0 args special class will fail. And if it does not, render me scared. :)

@kayhayen

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Checkout this:

File "../../nuitka/nodes/ConstantRefNodes.py", line 996, in makeConstantRefNode
assert False, (constant, constant_type)
AssertionError: (<zip object at 0x7fac6ced8408>, <class 'zip'>)

https://nightshift.nuitka.net/#/builders/176/builds/15

There you have it...

@bksahu bksahu force-pushed the bksahu:optimize_zip branch from 00adc66 to c7ff21d Aug 22, 2019

@bksahu bksahu force-pushed the bksahu:optimize_zip branch from 31f3c04 to 38479ea Sep 4, 2019

@bksahu bksahu force-pushed the bksahu:optimize_zip branch from 38479ea to 322540d Sep 6, 2019

bksahu added 3 commits Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.