Skip to content

Update copy.py #104570

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

Closed
wants to merge 1 commit into from
Closed

Update copy.py #104570

wants to merge 1 commit into from

Conversation

rajucomp
Copy link

I think that some code was being repeated because of an if else condition. I tried to combine them into one. Would loved to know the thoughts of the community.

I think that some code was being repeated because of an if else condition. I tried to combine them into one. Would loved to know the thoughts of the community.
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link

ghost commented May 17, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@sunmy2019
Copy link
Member

What we have done here is called https://en.wikipedia.org/wiki/Loop_unswitching

It may cause a performance penalty. Have you done benchmarks?

Copy link

@Abbasi393 Abbasi393 left a comment

Choose a reason for hiding this comment

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

LGTM

@Eclips4
Copy link
Member

Eclips4 commented May 17, 2023

Hello!
Please, provide an results of benchmarks after(and before) this change.
If performance difference is not significantly, core developers doesn't merge this PR.
CPython core developers as a rule, doesn't merge cosmetic changes.

@AlexWaygood
Copy link
Member

@arhadthedev, please don't add "skip issue" and "skip news" to PRs like this:

  • Requiring an issue means that contributors need to state a clear rationale for their change, in words. Reviewing PRs is much easier if a contributor has clearly explained, in words, why the change is desirable.
  • Requiring a NEWS entry means that contributors need to explain the impact that their change has on users of the API that they're changing:
    • If there's no impact to users, then the change probably doesn't need to be made at all, and the PR should be closed.
    • If there is an impact to users, then the users deserve to know about it, so there should be a NEWS entry.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 18, 2023

@rajucomp, thank you for the PR! I agree that the code looks slightly simpler and less repetitive this way. However, I'm going to reject this PR, for three reasons:

  1. I can't find any performance difference from this PR. I've tried the pyperformance deepcopy benchmark, and a number of microbenchmarks, locally, and this PR doesn't seem to impact the execution time of any of them, unfortunately. If you're able to construct a benchmark that this PR improves, please post it here, and I'll be happy to reconsider :)

  2. CPython generally does not accept PRs that only make "cosmetic" changes to code. This is because any change, no matter how small, carries with it a certain risk of causing breakage -- and breakages in the standard library have a very high impact on users of Python. It's also to avoid unnecessary code churn and make git blame easy to read.

  3. Although there doesn't seem to be a real performance difference either way, it is more obvious from reading the existing (more verbose) code that it will have optimal performance. With your patch, you would do this, which would involve doing the if deep: check with every iteration of the for item in listiter loop:

    for item in listiter:
        if deep:
            item = deepcopy(item, memo)
        y.append(item)

    Whereas with the existing (more verbose code), we only have to do the if deep: check once. In practical terms, as I say, it doesn't seem to make a performance difference -- but it is "more obviously correct" for human readers of the code, even though it is more verbose and might seem more repetitive:

    if deep:
        for item in listiter:
            item = deepcopy(item, memo)
            y.append(item)
    else:
        for item in listiter:
            y.append(item)

Thanks again for the PR, though, and for your interest in improving CPython!

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

Successfully merging this pull request may close these issues.

7 participants