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

[mypyc] Improve support for compiling singledispatch #10795

Merged

Conversation

pranavrajpal
Copy link
Contributor

This makes several improvements to the support for compiling singledispatch that was introduced in #10753 by:

  • Making sure registered implementations defined later in a file take precedence when multiple overlap (33798df)
  • Using non-native calls to registered implementations to allow for adding other decorators to registered functions (099b047)
  • Creating a separate function that dispatches to the correct implementation instead of adding code to dispatch to one of the registered implementations directly into the main singledispatch function, allowing the main singledispatch function to be a generator (59555e4)
  • Avoiding a compilation error when trying to dispatch on an ABC (2d40421)

Test Plan

I extended the testErrorOutput test in commandline.test, added a few tests to run-singledispatch.test, and changed some of the existing tests in run-singledispatch.test from xfails to regular tests.

Going through the registered implementations in the opposite order that
they were defined means that implementations that are defined later in a
file are used instead of the ones earlier in the file when they overlap.
Registered implementations are always decorated because they always have
a register decorator, and we can't make native calls to decorated
functions because otherwise we wouldn't be able to use the function
emitted from the decorator (which would be a Python callable object).

Previously, we removed the register decorator on those implementations
and treated them as non-decorated functions, but that strategy stops
working as soon as you add another decorator, so this just treats those
implementations as regular decorated functions and uses a non-native
call for those functions.
Instead of adding the logic to dispatch to the correct function to the
main singledispatch function itself, generate 2 separate functions: one
that compiles the logic of the undecorated function normally, and a
separate dispatch function that checks the first argument's type and
dispatches either to one of the registered implementations or the
fallback.

That makes it easier to generate code when the main singledispatch
function is a generator or some other kind of special function, as the
code generation can generate the main singledispatch function the same
way it would otherwise.

Because we're no longer changing how that main singledispatch function
is generated based on whether it's a singledispatch function, we don't
need to store whether the current function is a singledispatch function
in FuncInfo.
This makes several changes to the singledispatch run tests:
- in testRegisterDoesntChangeFunction, pass the type as an argument to
  register to avoid a mypy error
- remove a test where we pass in an invalid type to a singledispatch
  function, because mypy correctly treats that as an error
- in testKeywordArguments, change the keyword argument type and return
  value to make it easier to tell what's failing
- add 2 new tests related to registering functions while applying other
  decorators to the registered functions
- change several previously xfailed tests to non-xfailed tests because
  they are now able to pass due to previous changes (like generating a
  separate dispatch function and using non-native calls)
When trying to use the Mapping ABC as the dispatch type for a registered
implementation, we previously tried to load the type for the isinstance
check as `typing.Mapping` even if it was imported from
`collections.abc`, causing a compilation error due to the fact that we
hadn't defined CPyModule_typing.

To fix that, this loads the type from the globals dict instead for most
types, and using the types in builtin_names for builtin types, which
won't be present in the globals dict.
Correctly supporting decorators being applied to functions after they've
been registered complicates the implementation because we wouldn't be
able to generate a call to the function that's been registered when
dispatching. Instead, we would have to:
- run all the decorators below the register decorator (we already do
  this)
- register the function at runtime by storing what the function looks
  like after those decorators have been run
- run the rest of the decorators

Doing that makes the implementation more complicated and having
decorators run after registering probably isn't common anyways, so we
just show an error.
Iterating over the decorators in reverse causes the numbers returned by
enumerate to be incorrect indices, which causes us to remove the wrong
decorators when using the indices in removed to delete elements from the
decorators_to_store list.
Move the functions that had decorators after registers to
commandline.test so that we can test that compilation errors are
correctly emitted. Change the remaining decorator-related tests in
run-singledispatch.test to make sure any decorators are applied to
functions before the functions are registered.
Correct several bugs in tests that allows them to start passing. Also
changes several xfail tests to regular tests because they are now able
to pass.
Add a newline to the end of commandline.test, and remove some trailing
whitespace in run-singledispatch.test.
Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

Looks good. Look into my one question and we can merge

mypyc/irbuild/function.py Show resolved Hide resolved
Move the creation of the FuncIR corresponding to the dispatch function
to a separate function. This makes gen_func_item cleaner because more of
the logic is in a separate function.
@msullivan msullivan merged commit 7d69ce2 into python:master Jul 12, 2021
@pranavrajpal pranavrajpal deleted the improve-mypyc-singledispatch-support branch July 12, 2021 19:38
sthagen added a commit to sthagen/python-mypy that referenced this pull request Jul 13, 2021
[mypyc] Improve support for compiling singledispatch (python#10795)
@JukkaL
Copy link
Collaborator

JukkaL commented Jul 13, 2021

It looks like this may have regressed performance a bit: https://github.com/mypyc/mypyc-benchmark-results/blob/master/reports/benchmarks/sum_tree_singledispatch.md

The trend of performance is a bit worrying -- from 1.14x to 1.00x. It would be good to investigate what is going on.

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

Successfully merging this pull request may close these issues.

3 participants