Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821353002:
Use flat_set in SurfaceAggregator. (Closed)
DescriptionUse flat_set in SurfaceAggregator.
The sets and maps are typically small or empty and created once. flat_set and
flat_map will have better performance for these workloads.
See the bug for histograms of sizes.
Reland of https://codereview.chromium.org/2823043002 with fix.
BUG=712295
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=jbauman@chromium.org
Review-Url: https://codereview.chromium.org/2821353002
Cr-Commit-Position: refs/heads/master@{#465324}
Committed: https://chromium.googlesource.com/chromium/src/+/1ac5a973f47b2052df313bab17da120fae22190e
Patch Set 1 : Original landing #Patch Set 2 : Fix #
Total comments: 3
Patch Set 3 : Iterator fix #
Messages
Total messages: 20 (9 generated)
Description was changed from ========== Use flat_set in SurfaceAggregator. The sets and maks are typically small or empty and created once. flat_set and flat_map will have better performance for these workloads. See the bug for histograms of sizes. BUG=712295 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Reland of https://codereview.chromium.org/2823043002 with fix. ========== to ========== Use flat_set in SurfaceAggregator. The sets and maks are typically small or empty and created once. flat_set and flat_map will have better performance for these workloads. See the bug for histograms of sizes. Reland of https://codereview.chromium.org/2823043002 with fix. BUG=712295 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=jbauman@chromium.org ==========
brettw@chromium.org changed reviewers: + jbauman@chromium.org
Reland of flat_set patch. PS1 is the original landing so look at the diffs between 1 & 2. The problem was that flat_set doesn't have stable iterators and one was being cached across updates. I looked for other similar uses and did not find any. I did change one other for loop to be a range-based one that I noticed when checking.
The CQ bit was checked by brettw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use flat_set in SurfaceAggregator. The sets and maks are typically small or empty and created once. flat_set and flat_map will have better performance for these workloads. See the bug for histograms of sizes. Reland of https://codereview.chromium.org/2823043002 with fix. BUG=712295 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=jbauman@chromium.org ========== to ========== Use flat_set in SurfaceAggregator. The sets and maps are typically small or empty and created once. flat_set and flat_map will have better performance for these workloads. See the bug for histograms of sizes. Reland of https://codereview.chromium.org/2823043002 with fix. BUG=712295 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=jbauman@chromium.org ==========
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2821353002/diff/20001/cc/surfaces/surface_agg... File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2821353002/diff/20001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:340: referenced_surfaces_.erase(referenced_surfaces_.find(surface_id)); Shouldn't this be base::Erase?
https://codereview.chromium.org/2821353002/diff/20001/cc/surfaces/surface_agg... File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2821353002/diff/20001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:340: referenced_surfaces_.erase(referenced_surfaces_.find(surface_id)); On 2017/04/18 17:48:15, danakj wrote: > Shouldn't this be base::Erase? Oh, or nvm I guess we only have EraseIf for flat_tree.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Iterator fix
The CQ bit was checked by brettw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2821353002/diff/20001/cc/surfaces/surface_agg... File cc/surfaces/surface_aggregator.cc (right): https://codereview.chromium.org/2821353002/diff/20001/cc/surfaces/surface_agg... cc/surfaces/surface_aggregator.cc:340: referenced_surfaces_.erase(referenced_surfaces_.find(surface_id)); On 2017/04/18 17:49:12, danakj wrote: > On 2017/04/18 17:48:15, danakj wrote: > > Shouldn't this be base::Erase? > > Oh, or nvm I guess we only have EraseIf for flat_tree. I thought base::Erase is really a better version of std::remove/remove_if. If you want to find one thing in a map and delete it, the iterator calls are still the right thing.
On Tue, Apr 18, 2017 at 2:26 PM, <brettw@chromium.org> wrote: > > https://codereview.chromium.org/2821353002/diff/20001/cc/ > surfaces/surface_aggregator.cc > File cc/surfaces/surface_aggregator.cc (right): > > https://codereview.chromium.org/2821353002/diff/20001/cc/ > surfaces/surface_aggregator.cc#newcode340 > cc/surfaces/surface_aggregator.cc:340: > referenced_surfaces_.erase(referenced_surfaces_.find(surface_id)); > On 2017/04/18 17:49:12, danakj wrote: > > On 2017/04/18 17:48:15, danakj wrote: > > > Shouldn't this be base::Erase? > > > > Oh, or nvm I guess we only have EraseIf for flat_tree. > > I thought base::Erase is really a better version of > std::remove/remove_if. If you want to find one thing in a map and delete > it, the iterator calls are still the right thing. > Right, ok that's a good way to think about them. > > https://codereview.chromium.org/2821353002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492539711088510,
"parent_rev": "952bde5d7daf1e3627e116ce6f2ed770ef0a4913", "commit_rev":
"1ac5a973f47b2052df313bab17da120fae22190e"}
Message was sent while issue was closed.
Description was changed from ========== Use flat_set in SurfaceAggregator. The sets and maps are typically small or empty and created once. flat_set and flat_map will have better performance for these workloads. See the bug for histograms of sizes. Reland of https://codereview.chromium.org/2823043002 with fix. BUG=712295 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=jbauman@chromium.org ========== to ========== Use flat_set in SurfaceAggregator. The sets and maps are typically small or empty and created once. flat_set and flat_map will have better performance for these workloads. See the bug for histograms of sizes. Reland of https://codereview.chromium.org/2823043002 with fix. BUG=712295 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=jbauman@chromium.org Review-Url: https://codereview.chromium.org/2821353002 Cr-Commit-Position: refs/heads/master@{#465324} Committed: https://chromium.googlesource.com/chromium/src/+/1ac5a973f47b2052df313bab17da... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1ac5a973f47b2052df313bab17da...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2826823002/ by ojan@chromium.org. The reason for reverting is: Broke cc_unittests SurfaceAggregatorWithResourcesTest.SecureOutputTexture. https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2... https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
