Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
135533002:
[Core] Optimize List (Closed)
|
Created:
6 years, 11 months ago by vicb Modified:
6 years, 10 months ago CC:
reviews_dartlang.org, Søren Gjesse Visibility:
Public. |
Description[Core] Optimize List
No need to check changes where the list could not change
BUG=
R=lrn@google.com
Committed: https://code.google.com/p/dart/source/detail?r=32425
Patch Set 1 #
Total comments: 7
Patch Set 2 : integrate feedback #
Total comments: 2
Patch Set 3 : integrate feedback #Messages
Total messages: 14 (0 generated)
Could someone tell me if there is a way to sell pending CLs related to Dart on codereview.chromium.org ? Thanks
Lasse, could you take a look at this?
Thanks Søren. My previous message should read "to see" rather than "to sell" ! (I want to *see* all pending CLs)
LGTM on the join change, no-go on the contains change. https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart File sdk/lib/collection/list.dart (right): https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart... sdk/lib/collection/list.dart:84: if (this[i] == element) return true; If the '==' operator on the elements modify the list, we want a concurrent modification error here. Ofcourse it shouldn't, but when it does, it's most likely a programming error, and we want to catch it soon. I am considering whether just changing the loop limit to list.length will enable better optimizations, but I don't want to avoid the error here. https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart... sdk/lib/collection/list.dart:166: return buffer.toString(); This change looks ok, if performance is as good (or better!) as before. My quick checks tracks it as pretty much performance neutral (checking join of 10000 long fixed-length List and Uint32List of the numbers 0..999 as string and as number).
Thanks for the review. It actually made of think about using 'assert's to make the prod code marginally faster, see inline comments. https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart File sdk/lib/collection/list.dart (right): https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart... sdk/lib/collection/list.dart:84: if (this[i] == element) return true; I haven't thought about operator overloading here... Good catch. However this would mean than both the element is container and it modify the list, should be pretty unusual. But still I agree the check should remain. What about an 'assert' so that prod code is not affected. If it's ok what about changing for assert for all the ConcurrentModificationError ? https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart... sdk/lib/collection/list.dart:166: return buffer.toString(); I haven't seen much of a change in perf - it should be *marginally* faster as the length check as been removed and the code is doing pretty much the same (inside writeAll)
https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart File sdk/lib/collection/list.dart (right): https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart... sdk/lib/collection/list.dart:84: if (this[i] == element) return true; Pretty unusual, hopefully. While we could probably wrap some modification checks in asserts, we couldn't do it for all of them. In some cases, a concurrent modification might mean that we can't continue iterating (e.g., a linked list where the links are no longer valid). I think I'll prefer to keep the checks in production mode too. But if we are doing them on fixed-length lists, we should fix that. https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart... sdk/lib/collection/list.dart:166: return buffer.toString(); I see marginally slower (very, very marginally), probably because it goes through an iterator instead of directly indexing into the list. I think it's still ok, though.
Lasse, I'll make the required modifs later today. https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart File sdk/lib/collection/list.dart (right): https://codereview.chromium.org/135533002/diff/1/sdk/lib/collection/list.dart... sdk/lib/collection/list.dart:84: if (this[i] == element) return true; On 2014/02/04 12:32:00, Lasse Reichstein Nielsen wrote: > I think I'll prefer to keep the checks in production mode too. Crashing because of the check or because of any other induced errors in not really different in prod ? I'll revert anyway > But if we are doing them on fixed-length lists, we should fix that. Don't get what you mean by this last sentence ?
I've reverted the incorrect hunk.
LGTM https://codereview.chromium.org/135533002/diff/120001/sdk/lib/collection/list... File sdk/lib/collection/list.dart (right): https://codereview.chromium.org/135533002/diff/120001/sdk/lib/collection/list... sdk/lib/collection/list.dart:165: if (separator.isEmpty) { Just checked the StringBuffer code: This "if" isn't necessary, you can just pass the separator directly to writeAll in all cases. (It starts by checking if separator.isEmpty itself, and has "" as default too).
latest feedback has been integrated https://codereview.chromium.org/135533002/diff/120001/sdk/lib/collection/list... File sdk/lib/collection/list.dart (right): https://codereview.chromium.org/135533002/diff/120001/sdk/lib/collection/list... sdk/lib/collection/list.dart:165: if (separator.isEmpty) { Indeed. Thank for the tip.
Excellent. I plan on landing this tomorrow.
Message was sent while issue was closed.
Committed patchset #3 manually as r32425 (presubmit successful). |
