GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-91432: Implement the FOR_ITER_SET specialization by corona10 · Pull Request #94104 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Jun 22, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Microbenchmark

result: Mean +- std dev: [base] 662 ns +- 5 ns -> [FOR_ITER_SET] 616 ns +- 2 ns: 1.07x faster

import pyperf

runner = pyperf.Runner()
runner.timeit(name="set iter",
              stmt="""
for e in s:
    pass
""",
              setup = """
s = set(range(100))
"""
              )

Leak test

➜  cpython git:(gh-91432) ✗ ./python.exe -m test test_set -R 3:3
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 7.62 Run tests sequentially
0:00:00 load avg: 7.62 [1/1] test_set
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 23.3 sec
Tests result: SUCCESS

I just follow @sweeneyde's work.
(Following the faster CPython project is kind of my daily hobby ;) Sorry if you already worked on your local branch.. )
According to his stat, set iteration (3.8%) would be worth optimizing as same as tuple iteration (2.8%)
This PR would be decided to be merged after #94096 is merged.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
corona10 force-pushed the gh-91432 branch from 575b0e8 to 1d6c74c Compare June 22, 2022 08:11
@bedevere-bot bedevere-bot added the awaiting core review label Jun 22, 2022
@corona10
corona10 requested a review from sweeneyde June 22, 2022 08:11
@corona10
Implement FOR_ITER_SET
5f147c6
@corona10
corona10 force-pushed the gh-91432 branch from 1d6c74c to 5f147c6 Compare June 22, 2022 08:12
@corona10 corona10 added the skip news label Jun 22, 2022
@corona10
nit
dba84eb
@corona10 corona10 changed the title GH-91432: Implement FOR_ITER_SET GH-91432: Implement the FOR_ITER_SET specialization Jun 22, 2022
@corona10
corona10 marked this pull request as ready for review June 22, 2022 08:56
@corona10
corona10 requested review from a team, markshannon and rhettinger as code owners June 22, 2022 08:56
corona10 added 3 commits June 22, 2022 17:57
@corona10
Remove unused variable
e8577bf
@corona10
Fix comment
1f7f882
@corona10
Use C99 designated initializer
f7f7305
@markshannon

markshannon commented Jun 22, 2022

Copy link
Copy Markdown
Member

Is iteration over sets common enough to justify this?
The stats on pyperformance suggest it is only ~3% of FOR_ITER executions.

Our main motivation for specializing FOR_ITER is to allow us to iterate over generators without making C calls and without slowing down iterating over sequences.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Jun 22, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@markshannon
Thanks, yeah stat looks changed from https://gist.github.com/sweeneyde/e1a7e98890e75c9355f99067b03ee37b.
(What makes change?)
dict keys and ascii string is more worth to specializing them.
It seems that all I can defend is that the numbers are lower than theirs(dict keys, ascii string) but are similar lol.

I am okay not applying this change if this is not worth specializing in the operation.
Can we run the performance benchmark? (Sorry, I don't have the physical machine to run this.. )
if there is no contribution to performance improvement, I am fine to not specialize it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon

markshannon commented Jun 22, 2022

Copy link
Copy Markdown
Member

The change in stats is presumably from merging #91713

I'll run pyperformance on this PR, but its in the queue behind #94109, so might take a while...
corona10 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 commented Jun 22, 2022
View reviewed changes
Comment thread Objects/setobject.c
Comment on lines +825 to 832
static PyObject *setiter_iternext(_PySetIterObject *si) {
PyObject *stack[1];
int err = _PySetIter_GetNext(si, stack);
if (err <= 0) {
return NULL;
}
return stack[0];
}

@corona10 corona10 Jun 22, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
static PyObject *setiter_iternext(_PySetIterObject *si) {
PyObject *stack[1];
int err = _PySetIter_GetNext(si, stack);
if (err <= 0) {
return NULL;
}
return stack[0];
}
static PyObject *setiter_iternext(_PySetIterObject *si) {
PyObject *key;
if (_PySetIter_GetNext(si, &key) <= 0) {
return NULL;
}
return key;
}

might be better

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markshannon

markshannon commented Jun 23, 2022

Copy link
Copy Markdown
Member

Performance shows a 1% speedup, although I think this just shows that 1% variations can be caused by code layout changes and other build effects.

Projecting from stats, FOR_ITER_SET would represent about 0.03% of executed instructions. (FOR_ITER_ADAPTIVE is 0.8% and of that only 4% are sets.)

I doubt that any further specialization of FOR_ITER is worthwhile.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Jun 23, 2022

Copy link
Copy Markdown
Member Author

@markshannon agree Let's close the PR!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 closed this Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sweeneyde sweeneyde Awaiting requested review from sweeneyde
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@corona10 @markshannon @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.