-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
operator.attrgetter slower than lambda after adding dotted names ability #54369
Comments
(Discovered in that StackOverflow answer: http://stackoverflow.com/questions/3940518/3942509#3942509 ; check the comments too) operator.attrgetter in its simplest form (i.e. with a single non-dotted name) needs more time to execute than an equivalent lambda expression. Attached file so3940518.py runs a simple benchmark comparing: a list comprehension of plain attribute access; attrgetter; and lambda. I will append sample benchmark times at the end of the comment. Browsing Modules/operator.c, I noticed that the dotted_getattr function was using PyUnicode_Check and (possibly) splitting on dots on *every* call of the attrgetter, which I thought to be most inefficient. I changed the py3k-daily-snapshot source to make the PyUnicode_Check calls in the attrgetter_new function; also, I modified the algorithm to pre-parse the operator.attrgetter functions for possible dots in the names, in order for the dotted_getattr function to become speedier. The only “drawback” is that now operator.attrgetter raises a TypeError on creation, not on subsequent calls of the attrgetter object; this shouldn't be a compatibility problem. However, I obviously had to update both Doc/library/operator.rst and Lib/test/test_operator.py . I am not sure whether I should attach a zip/tar file with both the attachments (the sample benchmark and the diff); so I'll attach the diff in a further comment. On the Ubuntu server 9.10 where I made the changes, I ran the so3940518.py sample benchmark before and after the changes. Run before the changes (third column is seconds, less is better): list comp 0.40999999999999925 1000000 Run after the changes: list comp 0.40000000000000036 1000000 |
Here comes the diff to Modules/operator.c, Doc/library/operator.rst and Lib/test/test_operator.py . As far as I could check, there are no leaks, but a more experienced eye in core development could not hurt. Also, obviously test_operatory.py passes all tests. Should this be accepted, I believe it should be backported to 2.7 (at least). I can do that, just let me know. |
Newer version of the diff, since I forgot some "if(0) fprintf" debug calls that shouldn't be there. |
An explanation to the changes. The old code kept the operator.itemgetter arguments in the ag->attr member. If the argument count (ag->nattrs) was 1, the single argument was kept; if more than 1, a tuple of the original arguments was kept. On every attrgetter_call call, if ag->nattrs was 1, dotted_getattr was called with the plain ag->attr as attribute name; if > 2, dotted_getattr was called for every one of the original arguments. Now, ag->attr is always a tuple, containing either dotless strings or tuples of dotless strings: operator.attrgetter("name1", "name2.name3", "name4") stores ("name1", ("name2", "name3"), "name4") in ag->attr. dotted_getattr accordingly chooses based on type (either str or tuple, ensured by attrgetter_new) whether to do a single access or a recursive one. |
Thanks for noticing this. I wasn't aware that it had slowed down after dotted name support had been added. Since it is a mild performance issue, I'm lowering the priority. Will take a further look when I get the chance. A first look at the patch shows that it is bigger than I expected. Isn't it possible to check for a dot on construction and just record the boolean so that a fast, non-splitting path can be used. I'm reluctant to make the code volume grow much for this function. It is already a bit voluminous for the minor convenience it offers. |
Voice of ignorance here: why can't this be implemented in the "naive" way one might in Python, use the existing string splitting algorithms of stringlib, but just leave it in __new__. |
Modules/operator.c grows by ~70 lines, most of it the setup code for ag->attr; also I loop twice over the args of attrgetter_new, choosing fast code that runs once per attrgetter creation than temporary data. Alex's suggestion to make use of Python-level functions to shorten the code of attrgetter_new could obviously work to decrease the source lines. I don't know how fast I would produce such a version if requested, though. Whatever the way attrgetter_new sets up the data, I would suggest that you keep the logic changes in general, i.e. set-up in attrgetter_new and keep a thinner dotted_getattr , since it avoids running the same checks and splitting over and over again for every attrgetter_call invocation. |
Uploading separate files with .py, .diff extensions that can be viewed in a browser is indeed the right thing to do.\ |
A newer version of the patch with the following changes:
|
Some comments about the patch:
if (nattrs <= 1) {
if (!PyArg_UnpackTuple(args, "attrgetter", 1, 1, &attr))
return NULL;
Other than that, looks quite worthwhile. |
Thank you very much, Antoine, for your review. My comments in reply:
Attached a corrected version of the patch according to Antoine's comments. |
The patch looks fine. You're overview of the process presented here in the tracker would make a nice comment in the code. If Antoine is happy with the latest patch, then it's ready to apply. |
The PyUnicode_GET_SIZE issue was still there, but I've fixed it and committed in r86036. Thanks for your contribution! |
This is not the proper place for it, but in the 3.2 and 2.7 news it is reported that “The multi-argument form of operator.attrgetter() function now runs slightly faster” while it should be “The multi-argument form of operator.attrgetter() function now runs slightly faster and the single-argument form runs much faster”. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: