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

Fix caster optimization regression introduced in #3650 #3659

Merged
merged 7 commits into from
Jan 31, 2022

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Jan 28, 2022

Description

So I thought about this some more, and I realized the difference between simple type and simple ancestors in the caster. A non-simple type is any ancestor who has any descendants who have multi-inheritance from C++ classes. The descendant itself is a simple type though, unless it also has descendants. This PR fixes a mistake made in #3650 where the descendants themselves can remain simple types until they are extended. Marking a type as non-simple adds some additional pointer checks in the casters and while this is an edge case, we should be careful not to undo the optimization here.

Ping @virtuald
TLDR: Figured out what the difference is between simple_type and simple_ancestors.

  1. A type can be simple if it has MI, but has no descendants.
  2. As soon as it gets PyBind11 descendants, it ceases to be simple.
    In otherwise, !simple_ancestors => !simple_type only if it has PyBind11 descendants.
    I figured this out by thinking about why the rec.multiple_inheritance pathway didn't cause issues before as long multiple_inheritance was specified. The difference being that the single base pathway failed to update the type of the parent to non-simple.

Suggested changelog entry:

* Fix previous PR.

(just a reminder to include both PR numbers)

@Skylion007 Skylion007 requested review from rwgk and henryiii January 28, 2022 14:47
@Skylion007 Skylion007 self-assigned this Jan 28, 2022
Copy link
Contributor

@virtuald virtuald left a comment

Choose a reason for hiding this comment

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

Your explanation sounds like it captures the subtlety that I wasn't sure I had completely understood.

Can you update the documentation for the simple_type and simple_ancestors? Having it in this PR is ok, but they're really sparse on detail so making them a bit more clear would be good IMO.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Makes sense to me, although I cannot claim that I have a full grasp of all subtleties.

Tagging @jagerman in hopes that he can confirm or correct.

tests/test_multiple_inheritance.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

LGTM

@Skylion007 Skylion007 merged commit 3a8d923 into pybind:master Jan 31, 2022
@Skylion007 Skylion007 deleted the fix-mi-st-optimization branch January 31, 2022 17:19
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 31, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Feb 2, 2022
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.

4 participants