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

Issue 3269041: sort with a key speed patch

Can't Edit
Can't Publish+Mail
Start Review
11 years, 10 months ago by stutzbach
11 years, 10 months ago
Antoine Pitrou

Patch Set 1 #

Total comments: 1

Patch Set 2 : Rearranged bits of code for better performance #

Total comments: 8

Patch Set 3 : More attempts to tweak performance #

Total comments: 2

Patch Set 4 : Changed some IFLTs into ISLT for speed #

Patch Set 5 : Improved performance by informing the compiler about how pointers are #

Patch Set 6 : Removed an unused variable #

Patch Set 7 : Improved performance by informing the compiler about how pointers are #

Patch Set 8 : Saved some vertical space #

Patch Set 9 : Removed some unnecessary variables #

Patch Set 10 : Reduced diff #

Patch Set 11 : Renamed start to next to be more descriptive #

Total comments: 1

Patch Set 12 : Removed unnecessary goto #

Patch Set 13 : Updated comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+325 lines, -335 lines) Patch
M Objects/listobject.c View 1 2 3 4 5 6 7 8 9 10 11 12 33 chunks +325 lines, -335 lines 2 comments Download


Total messages: 1
Antoine Pitrou
11 years, 10 months ago (2010-11-23 21:44:05 UTC) #1
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.

File Objects/listobject.c (right):

Objects/listobject.c:1446: merge_freemem(ms);          /* reset to sane state */
Is there any reason to remove this?

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.

Objects/listobject.c:1099: Returns -1 in case of error.
Same comment about explicit inlining.

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?

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).

Objects/listobject.c:1787: * until the stack invariants are re-established:
Same comment about explicit inlining (!).

Objects/listobject.c:1816: }
Same comment about explicit inlining (!) :-)))

Objects/listobject.c:1858: r |= n & 1;
Any point in this change? Since you are requesting inlining I don't understand
what it brings.

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)

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.

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)

Objects/listobject.c:1036: /* assert [lo, start) is sorted */
Seems to me that "hi" was more descriptive here.

Objects/listobject.c:1447: return -1;
What is "pa" here?

Objects/listobject.c:1572: Fail:
Again, what is "pa"?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b