[base] Template MatchFun in TemplateHashMapImpl

[base] Template MatchFun in TemplateHashMapImpl

Make MatchFun a template parameter in TemplateHashMapImpl, moving the
PointersMatch function down to an implementation which extends
TemplateHashMapImpl to void* key and value (i.e. the same as the current
HashMap and ZoneHashMap typedefs).

This will allow other instantiations of TemplateHashMapImpl, with
different MatchFun values, e.g. std::equal_to, to have their key
equality test inlined, rather than calling a function pointer,

Committed: https://crrev.com/837c91e87bbaaa046e94ad62d924059324a0c10d
Cr-Commit-Position: refs/heads/master@{#39868}

[Leszek Swirski, 2016-09-19 16:12:24]

leszeks@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[Leszek Swirski, 2016-09-19 16:12:31]

The CQ bit was checked by leszeks@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 16:12:37]

Dry run: 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, 2016-09-19 16:21:47]

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

[commit-bot: I haz the power, 2016-09-19 16:21:48]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24368)

[rmcilroy, 2016-09-19 17:21:35]

looks good, a couple of comments

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h#newcode39
src/base/hashmap.h:39: TemplateHashMapImpl(MatchFun match = MatchFun(),
Does this need to be passed as a argument (and stored as a field) if it is being
templated? Could we just directly call the templated function instead (maybe
this is related to the code size thing you were discussing before)?

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h#newcode318
src/base/hashmap.h:318: template <typename AllocationPolicy>
Any reason we need both this class and TemplateHashMap - could we just inline
this code into TemplateHashMap and then:
typedef TemplateHashMap<void*, void*, DefaultAllocationPolicy> HashMap;
?

[Leszek Swirski, 2016-09-20 10:55:48]

The CQ bit was checked by leszeks@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 10:55:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Leszek Swirski, 2016-09-20 11:14:30]

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h#newcode39
src/base/hashmap.h:39: TemplateHashMapImpl(MatchFun match = MatchFun(),
On 2016/09/19 17:21:35, rmcilroy wrote:
> Does this need to be passed as a argument (and stored as a field) if it is
being
> templated? Could we just directly call the templated function instead (maybe
> this is related to the code size thing you were discussing before)?

Yes, because it could be stateful (not least, the function pointer instantiation
by PointerHashMap).

Regarding space, we could actually potentially shove this, the allocator, and
the Entry* map_ in a tuple, which would collapse any empty classes using the
empty-base-optimisation where possible (afaik it uses metaprogramming magic to
detect empty non-final types).

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h#newcode318
src/base/hashmap.h:318: template <typename AllocationPolicy>
On 2016/09/19 17:21:35, rmcilroy wrote:
> Any reason we need both this class and TemplateHashMap - could we just inline
> this code into TemplateHashMap and then:
> typedef TemplateHashMap<void*, void*, DefaultAllocationPolicy> HashMap;
> ?

It's to have

a) a static PointersMatch method, and
b) a constructor which does not default initialise MatchFun.
c) a common base for HashMap, TemplateHashMap and ZoneHashMap

The last one could be achieved with a templated using directive I guess, e.g.

template <typename AllocationPolicy>
using PointerHashMap = TemplateHashMap<void*, void*, AllocationPolicy>

but I think the first two are valid.

[commit-bot: I haz the power, 2016-09-20 11:35:37]

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

[commit-bot: I haz the power, 2016-09-20 11:35:38]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-09-20 12:57:57]

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h#newcode39
src/base/hashmap.h:39: TemplateHashMapImpl(MatchFun match = MatchFun(),
On 2016/09/20 11:14:30, Leszek Swirski wrote:
> On 2016/09/19 17:21:35, rmcilroy wrote:
> > Does this need to be passed as a argument (and stored as a field) if it is
> being
> > templated? Could we just directly call the templated function instead (maybe
> > this is related to the code size thing you were discussing before)?
> 
> Yes, because it could be stateful (not least, the function pointer
instantiation
> by PointerHashMap).
> 
> Regarding space, we could actually potentially shove this, the allocator, and
> the Entry* map_ in a tuple, which would collapse any empty classes using the
> empty-base-optimisation where possible (afaik it uses metaprogramming magic to
> detect empty non-final types).

Ok sounds good. No need to specialize for space, this seems fine.

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h#newcode318
src/base/hashmap.h:318: template <typename AllocationPolicy>
On 2016/09/20 11:14:30, Leszek Swirski wrote:
> On 2016/09/19 17:21:35, rmcilroy wrote:
> > Any reason we need both this class and TemplateHashMap - could we just
inline
> > this code into TemplateHashMap and then:
> > typedef TemplateHashMap<void*, void*, DefaultAllocationPolicy> HashMap;
> > ?
> 
> It's to have
> 
> a) a static PointersMatch method, and
> b) a constructor which does not default initialise MatchFun.
> c) a common base for HashMap, TemplateHashMap and ZoneHashMap
> 
> The last one could be achieved with a templated using directive I guess, e.g.
> 
> template <typename AllocationPolicy>
> using PointerHashMap = TemplateHashMap<void*, void*, AllocationPolicy>
> 
> but I think the first two are valid.

Ok makes sense. A couple of points though: 
 - This CL adds the default MatchFunc initialization to the constructor above -
is there any reason you did this if you are adding this class to avoid it?
 - Most users of HashMap just want to default PointersMatch function - would it
make sense to just make that a default argument to the constructor here and
remove the extra arguments in all the callers? 
 - The name seems somehow more specific than TemplateHashMap, even although this
is the super class. Could we call it TemplatePointerHashMapImpl or similar (it
shouldn't be used except for to typedef HashMap and ZoneHashMap anyway I don't
think.

One other thought (not sure if it would work) - could we add this code to
TemplateHashMap, and then typedef HashMap to be TemplateHashMap<void*, void*,
DefaultAllocationPolicy> HashMap. This would save us the extra class, and all
HashMap's would get an iterator if they wanted it. WDYT?

[Leszek Swirski, 2016-09-20 15:04:20]

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h#newcode39
src/base/hashmap.h:39: TemplateHashMapImpl(MatchFun match = MatchFun(),
On 2016/09/20 12:57:57, rmcilroy wrote:
> On 2016/09/20 11:14:30, Leszek Swirski wrote:
> > On 2016/09/19 17:21:35, rmcilroy wrote:
> > > Does this need to be passed as a argument (and stored as a field) if it is
> > being
> > > templated? Could we just directly call the templated function instead
(maybe
> > > this is related to the code size thing you were discussing before)?
> > 
> > Yes, because it could be stateful (not least, the function pointer
> instantiation
> > by PointerHashMap).
> > 
> > Regarding space, we could actually potentially shove this, the allocator,
and
> > the Entry* map_ in a tuple, which would collapse any empty classes using the
> > empty-base-optimisation where possible (afaik it uses metaprogramming magic
to
> > detect empty non-final types).
> 
> Ok sounds good. No need to specialize for space, this seems fine.

Acknowledged.

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h#newcode318
src/base/hashmap.h:318: template <typename AllocationPolicy>
On 2016/09/20 12:57:57, rmcilroy wrote:
> On 2016/09/20 11:14:30, Leszek Swirski wrote:
> > On 2016/09/19 17:21:35, rmcilroy wrote:
> > > Any reason we need both this class and TemplateHashMap - could we just
> inline
> > > this code into TemplateHashMap and then:
> > > typedef TemplateHashMap<void*, void*, DefaultAllocationPolicy> HashMap;
> > > ?
> > 
> > It's to have
> > 
> > a) a static PointersMatch method, and
> > b) a constructor which does not default initialise MatchFun.
> > c) a common base for HashMap, TemplateHashMap and ZoneHashMap
> > 
> > The last one could be achieved with a templated using directive I guess,
e.g.
> > 
> > template <typename AllocationPolicy>
> > using PointerHashMap = TemplateHashMap<void*, void*, AllocationPolicy>
> > 
> > but I think the first two are valid.
> 
> Ok makes sense. A couple of points though: 
>  - This CL adds the default MatchFunc initialization to the constructor above
-
> is there any reason you did this if you are adding this class to avoid it?

I was actually considering re-ordering the parameters in the TemplateHashMapImpl
to move MatchFun to the back, since it's likely to be default initialised, but
then I didn't and now this one makes less sense I guess. I could do that?

>  - Most users of HashMap just want to default PointersMatch function - would
it
> make sense to just make that a default argument to the constructor here and
> remove the extra arguments in all the callers? 

Done, which justifies the extra constructor a lot more ;)

>  - The name seems somehow more specific than TemplateHashMap, even although
this
> is the super class. Could we call it TemplatePointerHashMapImpl or similar (it
> shouldn't be used except for to typedef HashMap and ZoneHashMap anyway I don't
> think.

Done.

> One other thought (not sure if it would work) - could we add this code to
> TemplateHashMap, and then typedef HashMap to be TemplateHashMap<void*, void*,
> DefaultAllocationPolicy> HashMap. This would save us the extra class, and all
> HashMap's would get an iterator if they wanted it. WDYT?

This wouldn't work with TemplateHashMap as-is, because it uses private
inheritance to hide the base class. Honestly, I'd be more in favour of removing
it entirely (it's used in exactly one place), and replacing it with a partial
specialisation of TemplateHashMapImpl for pointers (with HashMap then maybe
being the void* typedef). But, again I'd want to do that in a separate CL to
keep changes fairly atomic.

[rmcilroy, 2016-09-20 16:06:04]

LGTM with comments addressed. Thanks

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2354593002/diff/1/src/base/hashmap.h#newcode318
src/base/hashmap.h:318: template <typename AllocationPolicy>
On 2016/09/20 15:04:20, Leszek Swirski wrote:
> On 2016/09/20 12:57:57, rmcilroy wrote:
> > On 2016/09/20 11:14:30, Leszek Swirski wrote:
> > > On 2016/09/19 17:21:35, rmcilroy wrote:
> > > > Any reason we need both this class and TemplateHashMap - could we just
> > inline
> > > > this code into TemplateHashMap and then:
> > > > typedef TemplateHashMap<void*, void*, DefaultAllocationPolicy> HashMap;
> > > > ?
> > > 
> > > It's to have
> > > 
> > > a) a static PointersMatch method, and
> > > b) a constructor which does not default initialise MatchFun.
> > > c) a common base for HashMap, TemplateHashMap and ZoneHashMap
> > > 
> > > The last one could be achieved with a templated using directive I guess,
> e.g.
> > > 
> > > template <typename AllocationPolicy>
> > > using PointerHashMap = TemplateHashMap<void*, void*, AllocationPolicy>
> > > 
> > > but I think the first two are valid.
> > 
> > Ok makes sense. A couple of points though: 
> >  - This CL adds the default MatchFunc initialization to the constructor
above
> -
> > is there any reason you did this if you are adding this class to avoid it?
> 
> I was actually considering re-ordering the parameters in the
TemplateHashMapImpl
> to move MatchFun to the back, since it's likely to be default initialised, but
> then I didn't and now this one makes less sense I guess. I could do that?

Yeah this probably makes sense - it does seem to be the least likely to be
default initialized looking through the code. Please reorder both
TemplateHashMapImpl and TemplatePointerHashMapImpl if doing this.

> >  - Most users of HashMap just want to default PointersMatch function - would
> it
> > make sense to just make that a default argument to the constructor here and
> > remove the extra arguments in all the callers? 
> 
> Done, which justifies the extra constructor a lot more ;)
> 
> >  - The name seems somehow more specific than TemplateHashMap, even although
> this
> > is the super class. Could we call it TemplatePointerHashMapImpl or similar
(it
> > shouldn't be used except for to typedef HashMap and ZoneHashMap anyway I
don't
> > think.
> 
> Done.
> 
> > One other thought (not sure if it would work) - could we add this code to
> > TemplateHashMap, and then typedef HashMap to be TemplateHashMap<void*,
void*,
> > DefaultAllocationPolicy> HashMap. This would save us the extra class, and
all
> > HashMap's would get an iterator if they wanted it. WDYT?
> 
> This wouldn't work with TemplateHashMap as-is, because it uses private
> inheritance to hide the base class. Honestly, I'd be more in favour of
removing
> it entirely (it's used in exactly one place), and replacing it with a partial
> specialisation of TemplateHashMapImpl for pointers (with HashMap then maybe
> being the void* typedef). But, again I'd want to do that in a separate CL to
> keep changes fairly atomic.

Right, hadn't noticed the private inheritance. Agreed - remove it entirely would
make more sense, but yeah, let's make this a separate CL.

https://codereview.chromium.org/2354593002/diff/60001/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2354593002/diff/60001/src/base/hashmap.h#newc...
src/base/hashmap.h:324: Base;
weird formating here - I guess this was git cl format. Any way you can wrangle
it into submission? (no worries if not).

https://codereview.chromium.org/2354593002/diff/60001/src/gdb-jit.cc
File src/gdb-jit.cc (right):

https://codereview.chromium.org/2354593002/diff/60001/src/gdb-jit.cc#newcode2019
src/gdb-jit.cc:2019: line_map = new base::HashMap(&);
remove "&" here

[Leszek Swirski, 2016-09-26 16:15:06]

The CQ bit was checked by leszeks@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 16:15:09]

Dry run: 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, 2016-09-26 16:44:54]

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

[commit-bot: I haz the power, 2016-09-26 16:44:55]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-09-26 16:54:52]

https://codereview.chromium.org/2354593002/diff/60001/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2354593002/diff/60001/src/base/hashmap.h#newc...
src/base/hashmap.h:324: Base;
On 2016/09/20 16:06:03, rmcilroy wrote:
> weird formating here - I guess this was git cl format. Any way you can wrangle
> it into submission? (no worries if not).

Not really, I could define the MatchFun typedef higher to do it but I don't want
Base to be public, so there'd be some weird interleaving of visibility modifiers
in that case.

https://codereview.chromium.org/2354593002/diff/60001/src/gdb-jit.cc
File src/gdb-jit.cc (right):

https://codereview.chromium.org/2354593002/diff/60001/src/gdb-jit.cc#newcode2019
src/gdb-jit.cc:2019: line_map = new base::HashMap(&);
On 2016/09/20 16:06:03, rmcilroy wrote:
> remove "&" here

Done.

[Leszek Swirski, 2016-09-28 13:18:57]

leszeks@chromium.org changed reviewers:
+ verwaest@chromium.org

[Leszek Swirski, 2016-09-28 13:18:57]

verwaest@chromium.org: Please review changes in

[Toon Verwaest, 2016-09-29 13:21:22]

lgtm profiler / snapshot, didn't look at all changes. I presume you need an
owner review given your incomplete sentence ;)

[Leszek Swirski, 2016-09-29 13:22:53]

The CQ bit was checked by leszeks@chromium.org

[Leszek Swirski, 2016-09-29 13:22:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2354593002/#ps80001
(title: "Remove bad search-replace")

[commit-bot: I haz the power, 2016-09-29 13:23:00]

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

[Leszek Swirski, 2016-09-29 13:23:28]

Perfect, and yes, sorry about that.

[commit-bot: I haz the power, 2016-09-29 13:53:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-29 13:53:31]

Description was changed from

==========
[base] Template MatchFun in TemplateHashMapImpl

Make MatchFun a template parameter in TemplateHashMapImpl, moving the
PointersMatch function down to an implementation which extends
TemplateHashMapImpl to void* key and value (i.e. the same as the current
HashMap and ZoneHashMap typedefs).

This will allow other instantiations of TemplateHashMapImpl, with
different MatchFun values, e.g. std::equal_to, to have their key
equality test inlined, rather than calling a function pointer,
==========

to

==========
[base] Template MatchFun in TemplateHashMapImpl

Make MatchFun a template parameter in TemplateHashMapImpl, moving the
PointersMatch function down to an implementation which extends
TemplateHashMapImpl to void* key and value (i.e. the same as the current
HashMap and ZoneHashMap typedefs).

This will allow other instantiations of TemplateHashMapImpl, with
different MatchFun values, e.g. std::equal_to, to have their key
equality test inlined, rather than calling a function pointer,

Committed: https://crrev.com/837c91e87bbaaa046e94ad62d924059324a0c10d
Cr-Commit-Position: refs/heads/master@{#39868}
==========

[commit-bot: I haz the power, 2016-09-29 13:53:33]

Patchset 5 (id:??) landed as
https://crrev.com/837c91e87bbaaa046e94ad62d924059324a0c10d
Cr-Commit-Position: refs/heads/master@{#39868}