KEMBAR78
[mypyc] Add a special case for str_rprimitive in builder.builtin_len by 97littleleaf11 · Pull Request #10710 · python/mypy · GitHub
Skip to content

[mypyc] Add a special case for str_rprimitive in builder.builtin_len #10710

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jul 1, 2021
Merged

[mypyc] Add a special case for str_rprimitive in builder.builtin_len #10710

merged 14 commits into from
Jul 1, 2021

Conversation

97littleleaf11
Copy link
Collaborator

Description

Closes mypyc/mypyc#835

  • Add a branch for str_rpimitive in builtin_len
  • Reduce redundant code
  • Faster list/tuple built from str

Test Plan

  • Add run tests for faster list/tuple built from str
  • Modify one irbuild test. The original one is redundant.

TH3CHARLie
TH3CHARLie reviewed Jun 25, 2021
View reviewed changes
97littleleaf11
97littleleaf11 commented Jun 28, 2021
View reviewed changes
mypyc/test-data/run-tuples.test
source_str = 'abbc'
b = tuple('s:' + x for x in source_str)
assert b == ('s:a', 's:b', 's:b', 's:c')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made a mistake last week that a cannot be assigned to Tuple[str...].

mypyc/test-data/irbuild-tuple.test

[case testTupleBuiltFromList2]
[case testTupleBuiltFromStr]
def f2(val: str) -> str:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is another testTupleBuiltFromList thus it's redundant.

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 28, 2021

Can you double check the perf impact by running some quick microbenchmark?

@97littleleaf11
Copy link
Collaborator Author

This branch:

interpreted: 0.000373s (avg of 2601 iterations; stdev 1.6%)
compiled:    0.000180s (avg of 2601 iterations; stdev 1.3%)

compiled is 2.070x faster

Master branch:

interpreted: 0.000357s (avg of 2675 iterations; stdev 4.6%)
compiled:    0.000233s (avg of 2675 iterations; stdev 2.2%)

compiled is 1.529x faster

Tested on:

def list_from_str() -> None:
    s1 = "aaaaa"
    n = 0
    for i in range(1000):
        list = [x for x in s1]
        n += len(list)
    assert n == 5000, n

@97littleleaf11
Copy link
Collaborator Author

Adding a wrapper function has little impact on performance, since the speed up is based on the optimized list comprehension:

interpreted: 0.000327s (avg of 2838 iterations; stdev 1.4%)
compiled:    0.000157s (avg of 2838 iterations; stdev 1.2%)

compiled is 2.084x faster

JukkaL
JukkaL reviewed Jun 30, 2021
View reviewed changes
JukkaL
JukkaL approved these changes Jul 1, 2021
View reviewed changes
Copy link
Collaborator

@JukkaL JukkaL 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! Thanks for the updates.

@JukkaL JukkaL merged commit 49bb90a into python:master Jul 1, 2021
@97littleleaf11 97littleleaf11 deleted the builtin-len-str branch July 6, 2021 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JukkaL JukkaL JukkaL approved these changes

+1 more reviewer

@TH3CHARLie TH3CHARLie TH3CHARLie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add specialization for strings in builtin_len

3 participants

@97littleleaf11 @JukkaL @TH3CHARLie