Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2838163002:
Use base::flat_set<> rather than std::set<> for ResourceType set (Closed)
|
Created:
3 years, 8 months ago by Adam Rice Modified:
3 years, 7 months ago Reviewers:
Nathan Parker CC:
chromium-reviews, vakh+watch_chromium.org, timvolodine Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse base::flat_set<> rather than std::set<> for ResourceType set
components::RemoteDatabaseManager keeps a set of ResourceTypes which it is
applied to. The members of the set are integers and there can be at most 18
members, so it is more efficient to use a base::flat_set here.
BUG=
Review-Url: https://codereview.chromium.org/2838163002
Cr-Commit-Position: refs/heads/master@{#468563}
Committed: https://chromium.googlesource.com/chromium/src/+/b08fe054f49f402b9e513ecc8283152f80eefb09
Patch Set 1 #
Total comments: 2
Patch Set 2 : Simpler initialisation for the flat_set with O(N^2) complexity #Patch Set 3 : Reserve the vector size upfront to reduce allocations #
Messages
Total messages: 24 (16 generated)
The CQ bit was checked by ricea@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.
ricea@chromium.org changed reviewers: + nparker@chromium.org
Just a drive-by. Should shave off a few bytes and cycles, but feel free to say "no" if you don't want it.
https://codereview.chromium.org/2838163002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/remote_database_manager.cc (right): https://codereview.chromium.org/2838163002/diff/1/components/safe_browsing_db... components/safe_browsing_db/remote_database_manager.cc:104: std::vector<content::ResourceType> types_to_check; This seems seems like overkill for 18 elements that are added once at startup. Could we simplify the code by adding it directly to the set? Or sizing the set a head of time?
What's the difference between set and flat_set? How does it scale? FYI there are lots of ~small ::sets in https://cs.chromium.org/search/?q=::set+file:%5Esrc/components/safe_browsing_... that might benefit. It'd be good to do performance testing of it though.
https://codereview.chromium.org/2838163002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/remote_database_manager.cc (right): https://codereview.chromium.org/2838163002/diff/1/components/safe_browsing_db... components/safe_browsing_db/remote_database_manager.cc:104: std::vector<content::ResourceType> types_to_check; On 2017/04/25 15:23:14, Nathan Parker wrote: > This seems seems like overkill for 18 elements that are added once at startup. > Could we simplify the code by adding it directly to the set? Or sizing the set > a head of time? I've tried adding directly to the set in patch set 2. I don't like adding overhead during startup, but the simplification is compelling. By my back-of-the-envelope calculation it roughly doubles the number of operations needed for a full set with all 18 items. It's still an order of magnitude cheaper to initialise that the old std::set.
On 2017/04/25 15:25:04, Nathan Parker wrote: > What's the difference between set and flat_set? How does it scale? I found the guidance in https://cs.chromium.org/chromium/src/base/containers/README.md useful. My interpretation is that where the set is initialised once, and strictly bounded at < 100 integers, flat_set is clearly better. In other cases I would do benchmarks (which in practice means I'd only bother for hotspots).
The CQ bit was checked by ricea@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.
The CQ bit was checked by ricea@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.
lgtm
The CQ bit was checked by ricea@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493699027596890,
"parent_rev": "56a5100afa68236b7c4e2c5e2cf6f92a79245792", "commit_rev":
"b08fe054f49f402b9e513ecc8283152f80eefb09"}
Message was sent while issue was closed.
Description was changed from ========== Use base::flat_set<> rather than std::set<> for ResourceType set components::RemoteDatabaseManager keeps a set of ResourceTypes which it is applied to. The members of the set are integers and there can be at most 18 members, so it is more efficient to use a base::flat_set here. BUG= ========== to ========== Use base::flat_set<> rather than std::set<> for ResourceType set components::RemoteDatabaseManager keeps a set of ResourceTypes which it is applied to. The members of the set are integers and there can be at most 18 members, so it is more efficient to use a base::flat_set here. BUG= Review-Url: https://codereview.chromium.org/2838163002 Cr-Commit-Position: refs/heads/master@{#468563} Committed: https://chromium.googlesource.com/chromium/src/+/b08fe054f49f402b9e513ecc8283... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b08fe054f49f402b9e513ecc8283... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
