GeezerVison

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

GeezerVison is showing github.com — bpo-37207: Use vertorcall for list() by encukou · Pull Request #18928 · python/cpython

Conversation

encukou commented Mar 11, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

This continues the list() part of #13930. The complete pull request is stalled on discussions around dicts, but list() should not be controversial. (Range was done in #18464 and I plan to open PRs for other parts if this is merged.)
I did two small changes on top of Mark's code.

https://bugs.python.org/issue37207

Sorry, something went wrong.

Uh oh!

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

All reactions

markshannon and others added 4 commits

March 11, 2020 15:56


          Use vector call for list()

83f4943


          Add news entry

c586da5


          Use Py_ssize_t for argument count

9d53274


          list_vectorcall: Only fill the list if an argument is given

0418c6b

the-knights-who-say-ni added the CLA signed label

Mar 11, 2020

bedevere-bot added the awaiting core review label

Mar 11, 2020

corona10 approved these changes

Mar 13, 2020

View reviewed changes

corona10 left a comment

Copy link

Copy Markdown

Member

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.

I just left nit comment, else LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Misc/NEWS.d/next/Core and Builtins/2019-06-09-10-54-31.bpo-37207.bLjgLS.rst Resolved

Uh oh!

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


          NEWS: Update ReST syntax

84e88ca

Co-Authored-By: Dong-hee Na <[email protected]>

corona10 requested changes

Mar 17, 2020

View reviewed changes

corona10 left a comment

Copy link

Copy Markdown

Member

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.

@encukou

Hi, I re-left the comment for this issue.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/listobject.c Outdated

		list_vectorcall(PyObject type, PyObject const*args,
		size_t nargsf, PyObject *kwnames)
		{
		if (kwnames && PyTuple_GET_SIZE(kwnames) != 0) {

corona10 Mar 17, 2020

Copy link

Copy Markdown

Member

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.

Since #18980 is merged.
We can replace the logic into

if (!_PyArg_NoKwnames("list", kwnames)) {
    return NULL;
}

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/listobject.c Outdated

		return NULL;
		}
		Py_ssize_t nargs = PyVectorcall_NARGS(nargsf);
		if (nargs > 1) {

corona10 Mar 17, 2020

Copy link

Copy Markdown

Member

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.

We can replace it into

if (!_PyArg_CheckPositional("list", nargs, 0, 1)) {
    return NULL;
}

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/listobject.c

		return NULL;
		}

		assert(PyType_Check(type));

corona10 Mar 17, 2020

Copy link

Copy Markdown

Member

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.

Can you move this assert into the top of the function body since this parameter is the 1st parameter?

Sorry, something went wrong.

Uh oh!

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

All reactions

encukou Mar 17, 2020

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.

I don't see why that's necessary.

Sorry, something went wrong.

Uh oh!

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

All reactions

corona10 requested a review from vstinner

March 17, 2020 09:39

encukou added 2 commits

March 17, 2020 14:18


          Merge in master to get _PyArg_NoKwnames

d1fcdbe


          Use helper functions to validate arguments

47aba6d

corona10 approved these changes

Mar 17, 2020

View reviewed changes

corona10 left a comment

Copy link

Copy Markdown

Member

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.

Thanks for the change :)
lgtm

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner commented Mar 18, 2020

Copy link

Copy Markdown

Member

@corona10 @encukou: Can one of you please run a benchmark?

@markshannon wrote "As an initial step, we can speed up calls to range, list and dict by about 30%." at https://bugs.python.org/issue37207#msg345077 but I would prefer to see directly results of a microbenchmarks.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner approved these changes

Mar 18, 2020

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

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.

LGTM. But it would be better if someone can benchmark this optimization ;-)

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added awaiting merge and removed awaiting core review labels

Mar 18, 2020

corona10 commented Mar 18, 2020

Copy link

Copy Markdown

Member

@vstinner
When testing with list((1, 2, 3, 4, 5)) the performance enhancement shown like this.

Mean +- std dev: [master] 909 ns +- 29 ns -> [pr-18927] 791 ns +- 43 ns: 1.15x faster (-13%)

It shows a similar enhancement to the collections to which this feature is applied.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Mar 18, 2020

Copy link

Copy Markdown

Member

Ok, 1.15x faster (-13%) and -118 nanoseconds looks worth it to me. list(iterable) is commonly used.

Again, LGTM ;-)

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner merged commit ce10554 into python:master

Mar 30, 2020

bedevere-bot removed the awaiting merge label

Mar 30, 2020

vstinner commented Mar 30, 2020

Copy link

Copy Markdown

Member

I merged the PR, thanks @markshannon, @encukou and @corona10 ;-) There was an interesting typo: "vertorcall call" :-)

All reactions

Sorry, something went wrong.

Uh oh!

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

encukou deleted the vertorcall-newlist branch

March 31, 2020 12:37

cmaloney mentioned this pull request

Oct 28, 2026

Update list() to use the existing freelist #160858

Closed

Reviewers

vstinner

vstinner approved these changes

corona10

corona10 approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

6 participants

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.

GeezerVison

Conversation

encukou commented Mar 11, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

encukou Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Mar 18, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 commented Mar 18, 2020

Uh oh!

vstinner commented Mar 18, 2020

Uh oh!

vstinner commented Mar 30, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

encukou commented Mar 11, 2020 •

edited by bedevere-bot

Loading

Uh oh!

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