cc: Replace some instances of unordered_map with base::flat_map.

cc: Replace some instances of unordered_map with base::flat_map.

This patch replaces some instances of unordered_map with flat_map. This
is done for cases where we can reason out that the number of elements
contained is small.

R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2847553002
Cr-Commit-Position: refs/heads/master@{#467722}
Committed: https://chromium.googlesource.com/chromium/src/+/c6237d1546aab8f12ac514775b1ae67dd0de56aa

[vmpstr, 2017-04-26 22:04:38]

Description was changed from

==========
cc: Replace some instances of unordered_map with base::flat_map.

This patch replaces some instances of unordered_map with flat_map. This
is done for cases where we can reason out that the number of elements
contained is small.

R=danakj@chromium.org
==========

to

==========
cc: Replace some instances of unordered_map with base::flat_map.

This patch replaces some instances of unordered_map with flat_map. This
is done for cases where we can reason out that the number of elements
contained is small.

R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-04-26 22:12:00]

Please take a look.

https://codereview.chromium.org/2847553002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2847553002/diff/1/cc/output/gl_renderer.h#new...
cc/output/gl_renderer.h:303: std::unordered_map<ProgramKey,
std::unique_ptr<Program>, ProgramKeyHash>
The ProgramKey here is pretty big. It does have an equality operator, but
writing an operator< for it is a bit awkward (especially since one of the
comparison operators compares a pointer). 

Is the guidance in these cases to bite the bullet and just write operator<?

[danakj, 2017-04-26 22:16:32]

LGTM

https://codereview.chromium.org/2847553002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2847553002/diff/1/cc/output/gl_renderer.h#new...
cc/output/gl_renderer.h:303: std::unordered_map<ProgramKey,
std::unique_ptr<Program>, ProgramKeyHash>
 2017/04/26 22:12:00, vmpstr wrote:
> The ProgramKey here is pretty big. It does have an equality operator, but
> writing an operator< for it is a bit awkward (especially since one of the
> comparison operators compares a pointer). 
> 
> Is the guidance in these cases to bite the bullet and just write operator<?

As long as it's consistent and transitive and such, I don't think that comparing
pointers is a problem.

However I don't think we really understood the performance of flat_map with
large keys. As long as you're considering that size when you look at the size of
the map, and not just the number of elements then it should be okay I think
though. You might wanna perftest to be sure for this one tho, it does seem like
new territory.

[vmpstr, 2017-04-26 22:54:54]

On 2017/04/26 22:16:32, danakj wrote:
> LGTM
> 
> https://codereview.chromium.org/2847553002/diff/1/cc/output/gl_renderer.h
> File cc/output/gl_renderer.h (right):
> 
>
https://codereview.chromium.org/2847553002/diff/1/cc/output/gl_renderer.h#new...
> cc/output/gl_renderer.h:303: std::unordered_map<ProgramKey,
> std::unique_ptr<Program>, ProgramKeyHash>
>  2017/04/26 22:12:00, vmpstr wrote:
> > The ProgramKey here is pretty big. It does have an equality operator, but
> > writing an operator< for it is a bit awkward (especially since one of the
> > comparison operators compares a pointer). 
> > 
> > Is the guidance in these cases to bite the bullet and just write operator<?
> 
> As long as it's consistent and transitive and such, I don't think that
comparing
> pointers is a problem.

Pointer comparison for < is a bit hairy from the standards perspective, in that
I think the result of this comparison are "unspecified", but that doesn't
preclude it from being consistent... It's harder to argue that it doesn't
preclude it from being transitive, but it's probably fine...

I'm defer changing this for later.

> 
> However I don't think we really understood the performance of flat_map with
> large keys. As long as you're considering that size when you look at the size
of
> the map, and not just the number of elements then it should be okay I think
> though. You might wanna perftest to be sure for this one tho, it does seem
like
> new territory.

We could generate the hash for it as part of the item itself (kind of like we do
with image keys) and then use < on that first, so that it would early out if the
hashes don't match.

[vmpstr, 2017-04-26 22:54:58]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2017-04-26 22:56:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-04-26 22:56:37]

On 2017/04/26 22:54:54, vmpstr wrote:
> On 2017/04/26 22:16:32, danakj wrote:
> > LGTM
> > 
> > https://codereview.chromium.org/2847553002/diff/1/cc/output/gl_renderer.h
> > File cc/output/gl_renderer.h (right):
> > 
> >
>
https://codereview.chromium.org/2847553002/diff/1/cc/output/gl_renderer.h#new...
> > cc/output/gl_renderer.h:303: std::unordered_map<ProgramKey,
> > std::unique_ptr<Program>, ProgramKeyHash>
> >  2017/04/26 22:12:00, vmpstr wrote:
> > > The ProgramKey here is pretty big. It does have an equality operator, but
> > > writing an operator< for it is a bit awkward (especially since one of the
> > > comparison operators compares a pointer). 
> > > 
> > > Is the guidance in these cases to bite the bullet and just write
operator<?
> > 
> > As long as it's consistent and transitive and such, I don't think that
> comparing
> > pointers is a problem.
> 
> Pointer comparison for < is a bit hairy from the standards perspective, in
that
> I think the result of this comparison are "unspecified", but that doesn't
> preclude it from being consistent... It's harder to argue that it doesn't
> preclude it from being transitive, but it's probably fine...
> 
> I'm defer changing this for later.
> 
> > 
> > However I don't think we really understood the performance of flat_map with
> > large keys. As long as you're considering that size when you look at the
size
> of
> > the map, and not just the number of elements then it should be okay I think
> > though. You might wanna perftest to be sure for this one tho, it does seem
> like
> > new territory.
> 
> We could generate the hash for it as part of the item itself (kind of like we
do
> with image keys) and then use < on that first, so that it would early out if
the
> hashes don't match.

oh we can just use this:
 "For templates greater, less, greater_equal, and less_equal, the
specializations for any pointer type yield a total order, even if the built-in
operators <, >, <=, >= do not."

[commit-bot: I haz the power, 2017-04-27 00:38:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 00:38:11]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vmpstr, 2017-04-27 16:32:02]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2017-04-27 16:32:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 17:45:11]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493310722087540, "parent_rev":
"2b156ccc00ebdb53389dc59917f60cf1d1dde6ac", "commit_rev":
"c6237d1546aab8f12ac514775b1ae67dd0de56aa"}

[commit-bot: I haz the power, 2017-04-27 17:45:21]

Description was changed from

==========
cc: Replace some instances of unordered_map with base::flat_map.

This patch replaces some instances of unordered_map with flat_map. This
is done for cases where we can reason out that the number of elements
contained is small.

R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Replace some instances of unordered_map with base::flat_map.

This patch replaces some instances of unordered_map with flat_map. This
is done for cases where we can reason out that the number of elements
contained is small.

R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2847553002
Cr-Commit-Position: refs/heads/master@{#467722}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6237d1546aab8f12ac514775b1a...
==========

[commit-bot: I haz the power, 2017-04-27 17:45:22]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/c6237d1546aab8f12ac514775b1a...

[danakj, 2017-04-27 20:03:36]

On Wed, Apr 26, 2017 at 6:56 PM, <vmpstr@chromium.org> wrote:

> On 2017/04/26 22:54:54, vmpstr wrote:
> > On 2017/04/26 22:16:32, danakj wrote:
> > > LGTM
> > >
> > > https://codereview.chromium.org/2847553002/diff/1/cc/
> output/gl_renderer.h
> > > File cc/output/gl_renderer.h (right):
> > >
> > >
> >
> https://codereview.chromium.org/2847553002/diff/1/cc/output/gl_renderer.h#
> newcode303
> > > cc/output/gl_renderer.h:303: std::unordered_map<ProgramKey,
> > > std::unique_ptr<Program>, ProgramKeyHash>
> > > 2017/04/26 22:12:00, vmpstr wrote:
> > > > The ProgramKey here is pretty big. It does have an equality
> operator, but
> > > > writing an operator< for it is a bit awkward (especially since one
> of the
> > > > comparison operators compares a pointer).
> > > >
> > > > Is the guidance in these cases to bite the bullet and just write
> operator<?
> > >
> > > As long as it's consistent and transitive and such, I don't think that
> > comparing
> > > pointers is a problem.
> >
> > Pointer comparison for < is a bit hairy from the standards perspective,
> in
> that
> > I think the result of this comparison are "unspecified", but that doesn't
> > preclude it from being consistent... It's harder to argue that it doesn't
> > preclude it from being transitive, but it's probably fine...
> >
> > I'm defer changing this for later.
> >
> > >
> > > However I don't think we really understood the performance of flat_map
> with
> > > large keys. As long as you're considering that size when you look at
> the
> size
> > of
> > > the map, and not just the number of elements then it should be okay I
> think
> > > though. You might wanna perftest to be sure for this one tho, it does
> seem
> > like
> > > new territory.
> >
> > We could generate the hash for it as part of the item itself (kind of
> like we
> do
> > with image keys) and then use < on that first, so that it would early
> out if
> the
> > hashes don't match.
>
> oh we can just use this:
> "For templates greater, less, greater_equal, and less_equal, the
> specializations for any pointer type yield a total order, even if the
> built-in
> operators <, >, <=, >= do not."
>

Oh interesting. I have a feeling all software ever would break if pointers
were not integers and thus not transitive for comparisons but who knows.


>
> https://codereview.chromium.org/2847553002/
>

-- 
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.