Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2176683003:
Use index instead of iterator into DisplayItemList in PaintController (Closed)
|
Created:
4 years, 5 months ago by Xianzhu Modified:
4 years, 5 months ago Reviewers:
pdr. CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse index instead of iterator into DisplayItemList in PaintController
he problem of an iterator is that it will be invalid when the vector
reallocates buffer. An index won't be invalid as long as we don't
add/remove any item before the index.
An examples that we had used indices before this CL is
DisplayItemIndicesByClientMap which maps DisplayItemClient to indices
of display items. Iterators don't work because the map is built when we
are adding items into the list. Once the list reallocates the underlying
buffer, all existing iterators might become invalid.
Another example is
m_underInvalidationCheckingBegin/m_underInvalidationCheckingEnd.
Before this CL we had to reset them in appendDebugDrawingAfterCommit()
when the list changes. (I had a hard time debugging a flaky crash caused
by an invalid iterator before adding the reset code.)
Because we have to use indices, if we use iterators we have to convert
between them.
The main purpose of this CL is for https://codereview.chromium.org/2176573004/
which requires us to find a cached chunk by index of a display item.
Otherwise we would add another place converting iterator to index.
This CL doesn't expose indices across public api boundary.
Run 1093 of ct (https://ct.skia.org/chromium_perf_runs/) showed no performance
impact.
BUG=596983
Committed: https://crrev.com/dbea1f7009644ea416e5e0129bc255b946a88f73
Cr-Commit-Position: refs/heads/master@{#407683}
Patch Set 1 #Patch Set 2 : - #
Dependent Patchsets: Messages
Total messages: 19 (10 generated)
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
Iterators can be nice at the api boundary because they let you return const iterators. Iterators also prevent bugs with mixing size_t, int, and unsigned. For these reasons I'd prefer if we switched to iterators, but you may have an example that really does require indices instead. Can you describe the example that is easier with indices a bit more? I wonder if we could add a function to the iterator class to make it easier for your example?
On 2016/07/25 22:09:12, pdr. wrote: > Iterators can be nice at the api boundary because they let you return const > iterators. Iterators also prevent bugs with mixing size_t, int, and unsigned. > For these reasons I'd prefer if we switched to iterators, but you may have an > example that really does require indices instead. Can you describe the example > that is easier with indices a bit more? I wonder if we could add a function to > the iterator class to make it easier for your example? The problem of an iterator is that it will be invalid when the vector reallocates buffer. An index won't be invalid as long as we don't add/remove any item before the index. An examples that we had used indices before this CL is DisplayItemIndicesByClientMap which maps DisplayItemClient to indices of display items. Iterators don't work because the map is built when we are adding items into the list. Once the list reallocates the underlying buffer, all existing iterators might become invalid. Another example is m_underInvalidationCheckingBegin/m_underInvalidationCheckingEnd. Before this CL we had to reset them in appendDebugDrawingAfterCommit() when the list changes. (I had a hard time debugging a flaky crash caused by an invalid iterator before adding the reset code.) Because we have to use indices, if we use iterators we have to convert between them. The main purpose of this CL is for https://codereview.chromium.org/2176573004/ which requires us to find a cached chunk by index of a display item. Otherwise we would add another place converting iterator to index. This CL doesn't expose indices across public api boundary.
Run 1093 of ct (https://ct.skia.org/chromium_perf_runs/) showed no performance impact.
On 2016/07/25 at 22:33:55, wangxianzhu wrote: > On 2016/07/25 22:09:12, pdr. wrote: > > Iterators can be nice at the api boundary because they let you return const > > iterators. Iterators also prevent bugs with mixing size_t, int, and unsigned. > > For these reasons I'd prefer if we switched to iterators, but you may have an > > example that really does require indices instead. Can you describe the example > > that is easier with indices a bit more? I wonder if we could add a function to > > the iterator class to make it easier for your example? > > The problem of an iterator is that it will be invalid when the vector reallocates buffer. An index won't be invalid as long as we don't add/remove any item before the index. > > An examples that we had used indices before this CL is DisplayItemIndicesByClientMap which maps DisplayItemClient to indices of display items. Iterators don't work because the map is built when we are adding items into the list. Once the list reallocates the underlying buffer, all existing iterators might become invalid. > > Another example is m_underInvalidationCheckingBegin/m_underInvalidationCheckingEnd. Before this CL we had to reset them in appendDebugDrawingAfterCommit() when the list changes. (I had a hard time debugging a flaky crash caused by an invalid iterator before adding the reset code.) > > Because we have to use indices, if we use iterators we have to convert between them. > > The main purpose of this CL is for https://codereview.chromium.org/2176573004/ which requires us to find a cached chunk by index of a display item. Otherwise we would add another place converting iterator to index. > > This CL doesn't expose indices across public api boundary. Thanks for the explanation. Can you add that to the change description? (Just copy-paste it) LGTM
Description was changed from ========== Use index instead of iterator into DisplayItemList in PaintController We mixed index and iterator in PaintController to access DisplayItems in DisplayItemList. Now use index everywhere to keep consistence, and sometimes for more convenience. (This is especially convenience in the next CL to handle cached chunks in cached subsequences). BUG=596983 ========== to ========== Use index instead of iterator into DisplayItemList in PaintController We mixed index and iterator in PaintController to access DisplayItems in DisplayItemList. Now use index everywhere to keep consistence, and sometimes for more convenience. (This is especially convenience in the next CL to handle cached chunks in cached subsequences). Run 1093 of ct (https://ct.skia.org/chromium_perf_runs/) showed no performance impact. BUG=596983 ==========
Description was changed from ========== Use index instead of iterator into DisplayItemList in PaintController We mixed index and iterator in PaintController to access DisplayItems in DisplayItemList. Now use index everywhere to keep consistence, and sometimes for more convenience. (This is especially convenience in the next CL to handle cached chunks in cached subsequences). Run 1093 of ct (https://ct.skia.org/chromium_perf_runs/) showed no performance impact. BUG=596983 ========== to ========== Use index instead of iterator into DisplayItemList in PaintController he problem of an iterator is that it will be invalid when the vector reallocates buffer. An index won't be invalid as long as we don't add/remove any item before the index. An examples that we had used indices before this CL is DisplayItemIndicesByClientMap which maps DisplayItemClient to indices of display items. Iterators don't work because the map is built when we are adding items into the list. Once the list reallocates the underlying buffer, all existing iterators might become invalid. Another example is m_underInvalidationCheckingBegin/m_underInvalidationCheckingEnd. Before this CL we had to reset them in appendDebugDrawingAfterCommit() when the list changes. (I had a hard time debugging a flaky crash caused by an invalid iterator before adding the reset code.) Because we have to use indices, if we use iterators we have to convert between them. The main purpose of this CL is for https://codereview.chromium.org/2176573004/ which requires us to find a cached chunk by index of a display item. Otherwise we would add another place converting iterator to index. This CL doesn't expose indices across public api boundary. Run 1093 of ct (https://ct.skia.org/chromium_perf_runs/) showed no performance impact. BUG=596983 ==========
On 2016/07/25 23:27:24, pdr. wrote: > On 2016/07/25 at 22:33:55, wangxianzhu wrote: > > On 2016/07/25 22:09:12, pdr. wrote: > > > Iterators can be nice at the api boundary because they let you return const > > > iterators. Iterators also prevent bugs with mixing size_t, int, and > unsigned. > > > For these reasons I'd prefer if we switched to iterators, but you may have > an > > > example that really does require indices instead. Can you describe the > example > > > that is easier with indices a bit more? I wonder if we could add a function > to > > > the iterator class to make it easier for your example? > > > > The problem of an iterator is that it will be invalid when the vector > reallocates buffer. An index won't be invalid as long as we don't add/remove any > item before the index. > > > > An examples that we had used indices before this CL is > DisplayItemIndicesByClientMap which maps DisplayItemClient to indices of display > items. Iterators don't work because the map is built when we are adding items > into the list. Once the list reallocates the underlying buffer, all existing > iterators might become invalid. > > > > Another example is > m_underInvalidationCheckingBegin/m_underInvalidationCheckingEnd. Before this CL > we had to reset them in appendDebugDrawingAfterCommit() when the list changes. > (I had a hard time debugging a flaky crash caused by an invalid iterator before > adding the reset code.) > > > > Because we have to use indices, if we use iterators we have to convert between > them. > > > > The main purpose of this CL is for https://codereview.chromium.org/2176573004/ > which requires us to find a cached chunk by index of a display item. Otherwise > we would add another place converting iterator to index. > > > > This CL doesn't expose indices across public api boundary. > > Thanks for the explanation. Can you add that to the change description? (Just > copy-paste it) > Done. > LGTM
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use index instead of iterator into DisplayItemList in PaintController he problem of an iterator is that it will be invalid when the vector reallocates buffer. An index won't be invalid as long as we don't add/remove any item before the index. An examples that we had used indices before this CL is DisplayItemIndicesByClientMap which maps DisplayItemClient to indices of display items. Iterators don't work because the map is built when we are adding items into the list. Once the list reallocates the underlying buffer, all existing iterators might become invalid. Another example is m_underInvalidationCheckingBegin/m_underInvalidationCheckingEnd. Before this CL we had to reset them in appendDebugDrawingAfterCommit() when the list changes. (I had a hard time debugging a flaky crash caused by an invalid iterator before adding the reset code.) Because we have to use indices, if we use iterators we have to convert between them. The main purpose of this CL is for https://codereview.chromium.org/2176573004/ which requires us to find a cached chunk by index of a display item. Otherwise we would add another place converting iterator to index. This CL doesn't expose indices across public api boundary. Run 1093 of ct (https://ct.skia.org/chromium_perf_runs/) showed no performance impact. BUG=596983 ========== to ========== Use index instead of iterator into DisplayItemList in PaintController he problem of an iterator is that it will be invalid when the vector reallocates buffer. An index won't be invalid as long as we don't add/remove any item before the index. An examples that we had used indices before this CL is DisplayItemIndicesByClientMap which maps DisplayItemClient to indices of display items. Iterators don't work because the map is built when we are adding items into the list. Once the list reallocates the underlying buffer, all existing iterators might become invalid. Another example is m_underInvalidationCheckingBegin/m_underInvalidationCheckingEnd. Before this CL we had to reset them in appendDebugDrawingAfterCommit() when the list changes. (I had a hard time debugging a flaky crash caused by an invalid iterator before adding the reset code.) Because we have to use indices, if we use iterators we have to convert between them. The main purpose of this CL is for https://codereview.chromium.org/2176573004/ which requires us to find a cached chunk by index of a display item. Otherwise we would add another place converting iterator to index. This CL doesn't expose indices across public api boundary. Run 1093 of ct (https://ct.skia.org/chromium_perf_runs/) showed no performance impact. BUG=596983 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use index instead of iterator into DisplayItemList in PaintController he problem of an iterator is that it will be invalid when the vector reallocates buffer. An index won't be invalid as long as we don't add/remove any item before the index. An examples that we had used indices before this CL is DisplayItemIndicesByClientMap which maps DisplayItemClient to indices of display items. Iterators don't work because the map is built when we are adding items into the list. Once the list reallocates the underlying buffer, all existing iterators might become invalid. Another example is m_underInvalidationCheckingBegin/m_underInvalidationCheckingEnd. Before this CL we had to reset them in appendDebugDrawingAfterCommit() when the list changes. (I had a hard time debugging a flaky crash caused by an invalid iterator before adding the reset code.) Because we have to use indices, if we use iterators we have to convert between them. The main purpose of this CL is for https://codereview.chromium.org/2176573004/ which requires us to find a cached chunk by index of a display item. Otherwise we would add another place converting iterator to index. This CL doesn't expose indices across public api boundary. Run 1093 of ct (https://ct.skia.org/chromium_perf_runs/) showed no performance impact. BUG=596983 ========== to ========== Use index instead of iterator into DisplayItemList in PaintController he problem of an iterator is that it will be invalid when the vector reallocates buffer. An index won't be invalid as long as we don't add/remove any item before the index. An examples that we had used indices before this CL is DisplayItemIndicesByClientMap which maps DisplayItemClient to indices of display items. Iterators don't work because the map is built when we are adding items into the list. Once the list reallocates the underlying buffer, all existing iterators might become invalid. Another example is m_underInvalidationCheckingBegin/m_underInvalidationCheckingEnd. Before this CL we had to reset them in appendDebugDrawingAfterCommit() when the list changes. (I had a hard time debugging a flaky crash caused by an invalid iterator before adding the reset code.) Because we have to use indices, if we use iterators we have to convert between them. The main purpose of this CL is for https://codereview.chromium.org/2176573004/ which requires us to find a cached chunk by index of a display item. Otherwise we would add another place converting iterator to index. This CL doesn't expose indices across public api boundary. Run 1093 of ct (https://ct.skia.org/chromium_perf_runs/) showed no performance impact. BUG=596983 Committed: https://crrev.com/dbea1f7009644ea416e5e0129bc255b946a88f73 Cr-Commit-Position: refs/heads/master@{#407683} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/dbea1f7009644ea416e5e0129bc255b946a88f73 Cr-Commit-Position: refs/heads/master@{#407683} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
