Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(4420)

#27574: Faster parsing keyword arguments

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 3 months ago by storchaka+cpython
Modified:
10 months, 1 week ago
Reviewers:
pitrou
CC:
brett.cannon, rhettinger, gregory.p.smith, AntoinePitrou, haypo, larry, devnull_psf.upfronthosting.co.za, storchaka
Visibility:
Public.

Patch Set 1 #

Total comments: 19

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Python/getargs.c View 1 2 3 5 chunks +139 lines, -142 lines 0 comments Download

Messages

Total messages: 2
AntoinePitrou
https://bugs.python.org/review/27574/diff/17914/Python/getargs.c File Python/getargs.c (right): https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode1826 Python/getargs.c:1826: parser->kwlist = tuple; If it's a tuple then calling ...
1 year, 2 months ago #1
storchaka
1 year, 2 months ago #2
Thank you for your review Antoine. I tried to make changes simple for easier
reviewing, but if if you are prefer I'll factor out format checking code into
parser_init().

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c
File Python/getargs.c (right):

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode1826
Python/getargs.c:1826: parser->kwlist = tuple;
On 2016/08/02 22:53:34, AntoinePitrou wrote:
> If it's a tuple then calling it `kwtuple` would be less confusing...

Done.

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode1859
Python/getargs.c:1859: assert(format != NULL);
On 2016/08/02 22:53:34, AntoinePitrou wrote:
> Isn't format undefined at this point?

Good catch.

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode1898
Python/getargs.c:1898: if (min != INT_MAX) {
On 2016/08/02 22:53:34, AntoinePitrou wrote:
> Why isn't the whole checking and computation of `min` and `max` done in
> parser_init()?

I were going to do this on next step. For now the code of vgetargskeywordsfast
is just a copy of vgetargskeywords with minimal changes. This should make
reviewing easier. If you prefer, I can start refactoring right now.

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode1940
Python/getargs.c:1940: if (IS_END_OF_FORMAT(*format)) {
On 2016/08/02 22:53:34, AntoinePitrou wrote:
> Ditto here.

Yes.

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode1946
Python/getargs.c:1946: if (!skip) {
On 2016/08/02 22:53:34, AntoinePitrou wrote:
> From this until the loop end the code is extremely similar to
vgetargskeywords()
> except for one function call, if I'm reading correctly.  You can probably
factor
> it out.
> 
> Another more radical approach is to rewrite vgetargskeywords() to create its
own
> parser, but perhaps that would make it slightly slower.

Since for now functions using vgetargskeywords is the only public API used in
third-party extensions I don't want to make it slower. I afraided that
refactoring can cause a regression (needed to pass many local variables by
reference). On other side, increasing the size of the code due its duplication
can have negative effect. We need to make precise measurements of all effects of
the refactoring.

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode1978
Python/getargs.c:1978: if (i < min) {
On 2016/08/02 22:53:34, AntoinePitrou wrote:
> I know this is copy-pasted from existing code but I'd welcome additional
> comments here to describe how these conditions occur (omitted optional
> arguments?).

Done.

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode2004
Python/getargs.c:2004: PyErr_Format(PyExc_SystemError, "%s: '%s'", msg,
On 2016/08/02 22:53:34, AntoinePitrou wrote:
> What's this? Should it go into parser_init() as well?

Yes.

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode2020
Python/getargs.c:2020: PyErr_Format(PyExc_SystemError,
On 2016/08/02 22:53:34, AntoinePitrou wrote:
> To parser_init()?

Yes.

https://bugs.python.org/review/27574/diff/17914/Python/getargs.c#newcode2281
Python/getargs.c:2281: Py_CLEAR(s->kwlist);
On 2016/08/02 22:53:34, AntoinePitrou wrote:
> A parser_clear() subroutine would be nicer IMHO.

Done.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7