Code review - Issue 3269041: sort with a key speed patchhttps://codereview.appspot.com/2010-11-23T21:44:05+00:00rietveld
Message from unknown
2010-11-23T02:31:31+00:00stutzbachurn:md5:e706cc184e9ced6156eaf438bd2b0852
Message from unknown
2010-11-23T02:31:59+00:00stutzbachurn:md5:fca782684fed48e83b9e001f37afcb0e
Message from unknown
2010-11-23T02:32:00+00:00stutzbachurn:md5:14594826fd7726ef6351b6229a9a41de
Message from unknown
2010-11-23T02:32:01+00:00stutzbachurn:md5:aa4e00c6b32cd099e37ae6caec6f906d
Message from unknown
2010-11-23T02:32:02+00:00stutzbachurn:md5:1025e1ca0d23662d3cbf4c2ddfe67312
Message from unknown
2010-11-23T02:32:04+00:00stutzbachurn:md5:f40d6499a55b93786d91369261721095
Message from unknown
2010-11-23T02:32:05+00:00stutzbachurn:md5:25462ee7a699393476960ebd72202d03
Message from unknown
2010-11-23T02:32:06+00:00stutzbachurn:md5:355aa3df5312604dd3142eb6e1e8b9d8
Message from unknown
2010-11-23T02:32:08+00:00stutzbachurn:md5:caea5bcf854c94131fa35bbc23eb731e
Message from unknown
2010-11-23T02:32:09+00:00stutzbachurn:md5:07bcd3dd9eef99ffb06a0e6b954e868d
Message from unknown
2010-11-23T02:32:10+00:00stutzbachurn:md5:6d5ec9462c2667de70094109dc957ef4
Message from unknown
2010-11-23T02:32:12+00:00stutzbachurn:md5:25e96d3fe56c261a2c4f877b61203f61
Message from unknown
2010-11-23T02:32:13+00:00stutzbachurn:md5:2f5122fdb8ded409d121dba9656377ce
Message from antoine.pitrou@gmail.com
2010-11-23T21:44:05+00:00Antoine Pitrouurn:md5:4145017b9ba527bafdf5061d06c6183c
Looks good generally, but many small things (see below).
Also, I find the splitting into incremental patches quite detrimental since the various patches are hardly independent.
http://codereview.appspot.com/3269041/diff/1/Objects/listobject.c
File Objects/listobject.c (right):
http://codereview.appspot.com/3269041/diff/1/Objects/listobject.c#newcode1446
Objects/listobject.c:1446: merge_freemem(ms); /* reset to sane state */
Is there any reason to remove this?
http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1027
Objects/listobject.c:1027: static int
I don't think there's any point in explicit inlining of such non-trivial routines. Just let the compiler do its job, IMO.
http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1099
Objects/listobject.c:1099: Returns -1 in case of error.
Same comment about explicit inlining.
http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1383
Objects/listobject.c:1383: ms->a.keys = ms->temparray;
A comment would be in order here. Are you trying to keep everything contiguous to maximize spatial locality?
http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1496
Objects/listobject.c:1496: sortslice_copy_incr(&dest, &ssb);
Please don't do such gratuitous style changes (especially given that the new style is worse than the old one).
http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1787
Objects/listobject.c:1787: * until the stack invariants are re-established:
Same comment about explicit inlining (!).
http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1816
Objects/listobject.c:1816: }
Same comment about explicit inlining (!) :-)))
http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1858
Objects/listobject.c:1858: r |= n & 1;
Any point in this change? Since you are requesting inlining I don't understand what it brings.
http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1922
Objects/listobject.c:1922:
This is starting to look cryptic (or at least non-obvious). Can you add a comment? (or switch back to the simpler stack_keys approach)
http://codereview.appspot.com/3269041/diff/4001/Objects/listobject.c#newcode1049
Objects/listobject.c:1049: * The second is vacuously true at the start.
This change looks gratuitous. Left-hand and right-hand code seem to do the exact same thing.
http://codereview.appspot.com/3269041/diff/4001/Objects/listobject.c#newcode1494
Objects/listobject.c:1494: if (k < 0)
Looks quite gratuitous too. Did you measure any significant improvement?
(ditto for the couple next changes in this patch)
http://codereview.appspot.com/3269041/diff/18001/Objects/listobject.c#newcode1036
Objects/listobject.c:1036: /* assert [lo, start) is sorted */
Seems to me that "hi" was more descriptive here.
http://codereview.appspot.com/3269041/diff/20001/Objects/listobject.c#newcode1447
Objects/listobject.c:1447: return -1;
What is "pa" here?
http://codereview.appspot.com/3269041/diff/20001/Objects/listobject.c#newcode1572
Objects/listobject.c:1572: Fail:
Again, what is "pa"?