Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2489113002:
Remove stl_util's deletion functions from MojoHostResolverImpl. (Closed)
|
Created:
4 years, 1 month ago by davidben Modified:
4 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove stl_util's deletion functions from MojoHostResolverImpl.
//net has many instances where a service object owns a set of Jobs on
behalf of consumers. Previously, a manually owned std::set<Job*> was
used. Some fixes have included:
- std::find_if on the std::set<std::unique_ptr<Job>>. This is the worst
of both worlds as it keeps std::set's overhead without ever getting
the asymptotics from it.
- std::map<Job*, std::unique_ptr<Job>>. This is comparable to the
original scheme, but there have been concerns about cache locality.
- std::find_if on a std::vector<std::unique_ptr<Job>>. This has good cache
locality, but does not scale as the number of requests go up.
Starting from first principles, the ideal structure here would be an
intrusive linked list, so a Job can remove itself on completion. This
avoids extra allocations of intermediate nodes in the std::map and even
has better asymptotics than the std::set.
The STL doesn't quite provide intrusive containers, but C++11 lets us
get pretty close. If we use a std::list<Job> (note no pointer) and only
add to the list via emplacement, Jobs need not be copyable and we embed
the std::list pointers inside the Job object. However we need to store
an extra member in the Job so it can find its own iterator. (This
iterator should ultimately be a pointer back to the struct embedding the
Job anyway.)
BUG=555865
Committed: https://crrev.com/86a6788070f1872e7136d6c1ebe0a0d35e4a2e1c
Cr-Commit-Position: refs/heads/master@{#437375}
Patch Set 1 #
Total comments: 4
Patch Set 2 : add missing include, rebase #
Messages
Total messages: 21 (11 generated)
The CQ bit was checked by davidben@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...
davidben@chromium.org changed reviewers: + avi@chromium.org, mmenke@chromium.org
I should mention that, although std::list does allocate a separate node for each entry, anything based on std::unique_ptr<Job> will also allocate a separate Job for each entry. By using std::list<Job> directly, the list node ends up with a copy of Job embedded in it directly and so the linked list node and Job objects share an allocation.
lgtm I'd be good with this.
Description was changed from ========== Remove stl_util's deletion functions from MojoHostResolverImpl. //net has many instances where a service object owns a set of Jobs on behalf of consumers. Previously, a manually owned std::set<Job*> was used. Some fixes have included: - std::find_if on the std::set<std::unique_ptr<Job>>. This is the worst of both worlds as it keeps std::set's overhead without ever getting the perfomance benefits from it. - std::map<Job*, std::unique_ptr<Job>>. This is comparable to the original scheme, but there have been concerns about cache locality. - std::find_if on a std::vector<std::unique_ptr<Job>>. This has good cache locality, but does not scale as the number of requests go up. Starting from first priniciples, the ideal structure here would be an intrusive linked list, so a Job can remove itself on completion. This avoids extra allocations of intermediate nodes in the std::map and even has better asymptotics than the std::set. The STL doesn't quite provide intrusive containers, but C++11 lets us get pretty close. If we use a std::list<Job> (note no pointer) and only add to the list via emplacement, Jobs need not be copyable and we embed the std::list pointers inside the Job object. However we need to store an extra member in the Job so it can find its own iterator. (This iterator should ultimately be a pointer back to the struct embedding the Job anyway.) BUG=555865 ========== to ========== Remove stl_util's deletion functions from MojoHostResolverImpl. //net has many instances where a service object owns a set of Jobs on behalf of consumers. Previously, a manually owned std::set<Job*> was used. Some fixes have included: - std::find_if on the std::set<std::unique_ptr<Job>>. This is the worst of both worlds as it keeps std::set's overhead without ever getting the asymptotics from it. - std::map<Job*, std::unique_ptr<Job>>. This is comparable to the original scheme, but there have been concerns about cache locality. - std::find_if on a std::vector<std::unique_ptr<Job>>. This has good cache locality, but does not scale as the number of requests go up. Starting from first principles, the ideal structure here would be an intrusive linked list, so a Job can remove itself on completion. This avoids extra allocations of intermediate nodes in the std::map and even has better asymptotics than the std::set. The STL doesn't quite provide intrusive containers, but C++11 lets us get pretty close. If we use a std::list<Job> (note no pointer) and only add to the list via emplacement, Jobs need not be copyable and we embed the std::list pointers inside the Job object. However we need to store an extra member in the Job so it can find its own iterator. (This iterator should ultimately be a pointer back to the struct embedding the Job anyway.) BUG=555865 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, some completely optional suggestions. Sorry I didn't get to this yesterday. https://codereview.chromium.org/2489113002/diff/1/net/dns/mojo_host_resolver_... File net/dns/mojo_host_resolver_impl.cc (right): https://codereview.chromium.org/2489113002/diff/1/net/dns/mojo_host_resolver_... net/dns/mojo_host_resolver_impl.cc:80: pending_jobs_.erase(job); Worth checking if &*job is in the list? I'm not really sure if it is. https://codereview.chromium.org/2489113002/diff/1/net/dns/mojo_host_resolver_... net/dns/mojo_host_resolver_impl.cc:100: DCHECK_EQ(this, &*iter_); optional: Maybe also do this in Job::OnConnectionError? Seems less regression prone, if we decide to not start jobs immediately or something, though suppose that's resolver_'s job. Could also toss one in OnResolveDone, just for kicks, though that does require going through this method.
https://codereview.chromium.org/2489113002/diff/1/net/dns/mojo_host_resolver_... File net/dns/mojo_host_resolver_impl.h (right): https://codereview.chromium.org/2489113002/diff/1/net/dns/mojo_host_resolver_... net/dns/mojo_host_resolver_impl.h:8: #include <list> While you're here, mind adding <memory>?
On 2016/11/11 16:18:04, mmenke wrote: > https://codereview.chromium.org/2489113002/diff/1/net/dns/mojo_host_resolver_... > File net/dns/mojo_host_resolver_impl.h (right): > > https://codereview.chromium.org/2489113002/diff/1/net/dns/mojo_host_resolver_... > net/dns/mojo_host_resolver_impl.h:8: #include <list> > While you're here, mind adding <memory>? davidben: ping
https://codereview.chromium.org/2489113002/diff/1/net/dns/mojo_host_resolver_... File net/dns/mojo_host_resolver_impl.h (right): https://codereview.chromium.org/2489113002/diff/1/net/dns/mojo_host_resolver_... net/dns/mojo_host_resolver_impl.h:8: #include <list> On 2016/11/11 16:18:03, mmenke wrote: > While you're here, mind adding <memory>? Done.
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2489113002/#ps20001 (title: "add missing include, rebase")
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": 20001, "attempt_start_ts": 1481231598857130,
"parent_rev": "cd54c85cc191ede1859fbcb9a4136ff2a24219c6", "commit_rev":
"4b94fa6f369df20627face2ef0460587f8a34e6c"}
Message was sent while issue was closed.
Description was changed from ========== Remove stl_util's deletion functions from MojoHostResolverImpl. //net has many instances where a service object owns a set of Jobs on behalf of consumers. Previously, a manually owned std::set<Job*> was used. Some fixes have included: - std::find_if on the std::set<std::unique_ptr<Job>>. This is the worst of both worlds as it keeps std::set's overhead without ever getting the asymptotics from it. - std::map<Job*, std::unique_ptr<Job>>. This is comparable to the original scheme, but there have been concerns about cache locality. - std::find_if on a std::vector<std::unique_ptr<Job>>. This has good cache locality, but does not scale as the number of requests go up. Starting from first principles, the ideal structure here would be an intrusive linked list, so a Job can remove itself on completion. This avoids extra allocations of intermediate nodes in the std::map and even has better asymptotics than the std::set. The STL doesn't quite provide intrusive containers, but C++11 lets us get pretty close. If we use a std::list<Job> (note no pointer) and only add to the list via emplacement, Jobs need not be copyable and we embed the std::list pointers inside the Job object. However we need to store an extra member in the Job so it can find its own iterator. (This iterator should ultimately be a pointer back to the struct embedding the Job anyway.) BUG=555865 ========== to ========== Remove stl_util's deletion functions from MojoHostResolverImpl. //net has many instances where a service object owns a set of Jobs on behalf of consumers. Previously, a manually owned std::set<Job*> was used. Some fixes have included: - std::find_if on the std::set<std::unique_ptr<Job>>. This is the worst of both worlds as it keeps std::set's overhead without ever getting the asymptotics from it. - std::map<Job*, std::unique_ptr<Job>>. This is comparable to the original scheme, but there have been concerns about cache locality. - std::find_if on a std::vector<std::unique_ptr<Job>>. This has good cache locality, but does not scale as the number of requests go up. Starting from first principles, the ideal structure here would be an intrusive linked list, so a Job can remove itself on completion. This avoids extra allocations of intermediate nodes in the std::map and even has better asymptotics than the std::set. The STL doesn't quite provide intrusive containers, but C++11 lets us get pretty close. If we use a std::list<Job> (note no pointer) and only add to the list via emplacement, Jobs need not be copyable and we embed the std::list pointers inside the Job object. However we need to store an extra member in the Job so it can find its own iterator. (This iterator should ultimately be a pointer back to the struct embedding the Job anyway.) BUG=555865 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove stl_util's deletion functions from MojoHostResolverImpl. //net has many instances where a service object owns a set of Jobs on behalf of consumers. Previously, a manually owned std::set<Job*> was used. Some fixes have included: - std::find_if on the std::set<std::unique_ptr<Job>>. This is the worst of both worlds as it keeps std::set's overhead without ever getting the asymptotics from it. - std::map<Job*, std::unique_ptr<Job>>. This is comparable to the original scheme, but there have been concerns about cache locality. - std::find_if on a std::vector<std::unique_ptr<Job>>. This has good cache locality, but does not scale as the number of requests go up. Starting from first principles, the ideal structure here would be an intrusive linked list, so a Job can remove itself on completion. This avoids extra allocations of intermediate nodes in the std::map and even has better asymptotics than the std::set. The STL doesn't quite provide intrusive containers, but C++11 lets us get pretty close. If we use a std::list<Job> (note no pointer) and only add to the list via emplacement, Jobs need not be copyable and we embed the std::list pointers inside the Job object. However we need to store an extra member in the Job so it can find its own iterator. (This iterator should ultimately be a pointer back to the struct embedding the Job anyway.) BUG=555865 ========== to ========== Remove stl_util's deletion functions from MojoHostResolverImpl. //net has many instances where a service object owns a set of Jobs on behalf of consumers. Previously, a manually owned std::set<Job*> was used. Some fixes have included: - std::find_if on the std::set<std::unique_ptr<Job>>. This is the worst of both worlds as it keeps std::set's overhead without ever getting the asymptotics from it. - std::map<Job*, std::unique_ptr<Job>>. This is comparable to the original scheme, but there have been concerns about cache locality. - std::find_if on a std::vector<std::unique_ptr<Job>>. This has good cache locality, but does not scale as the number of requests go up. Starting from first principles, the ideal structure here would be an intrusive linked list, so a Job can remove itself on completion. This avoids extra allocations of intermediate nodes in the std::map and even has better asymptotics than the std::set. The STL doesn't quite provide intrusive containers, but C++11 lets us get pretty close. If we use a std::list<Job> (note no pointer) and only add to the list via emplacement, Jobs need not be copyable and we embed the std::list pointers inside the Job object. However we need to store an extra member in the Job so it can find its own iterator. (This iterator should ultimately be a pointer back to the struct embedding the Job anyway.) BUG=555865 Committed: https://crrev.com/86a6788070f1872e7136d6c1ebe0a0d35e4a2e1c Cr-Commit-Position: refs/heads/master@{#437375} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/86a6788070f1872e7136d6c1ebe0a0d35e4a2e1c Cr-Commit-Position: refs/heads/master@{#437375} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
