Replace base::HashPair macros with a templated function.

Replace base::HashPair macros with a templated function.

Committed: https://crrev.com/07ece06c99c9b15c5d2ea779239f856e066c97da
Cr-Commit-Position: refs/heads/master@{#361299}

[Anand Mistry (off Chromium), 2015-11-16 02:11:43]

Description was changed from

==========
Experiment.

BUG=
==========

to

==========
Replace base::HashPair macros with a templated function.

Also, change base::Location's hash function to cast file_name to a unitptr_t
instead of uint64_t. This is expected to generate a call to base::HashInts32 on
a 32-bit platform.
==========

[Anand Mistry (off Chromium), 2015-11-16 02:22:14]

amistry@chromium.org changed reviewers:
+ danakj@chromium.org

[Anand Mistry (off Chromium), 2015-11-16 02:22:15]

I'd like to propose this change. Hopefully it isn't too offensive :P.

The motivation is that base::HashInts64 is a fairly hot function because it is
called by TrackedObjects::TallyABirthIfActive (one of the hottest according to
chromeos profiling). However, on a 32-bit platform using HashInts64 on 32-bit
values doesn't buy us any additional entropy (*hand-wavey*) compared to using
HashInts32. It is more expensive, generating an extra 4 64-bit multiplies, each
results in a function call (to _allmul) on Windows-32. Oh, and base::HashInts64
has many crash reports on x86, but none on x86-64. No idea why, since the
generated code looks quite reasonable. Probably bad machines.

Now, this cl has two separable parts. Changing base::HashPair to be a templated
function, which should make it less brittle to type definitions. And changing
the cast in base::Location to be of the "correct" size. If you want, I can split
this change into two.

[danakj, 2015-11-16 19:37:33]

On Sun, Nov 15, 2015 at 6:22 PM, <amistry@chromium.org> wrote:

> Reviewers: danakj (behind on reviews)
> CL: https://codereview.chromium.org/1445013002/
>
> Message:
> I'd like to propose this change. Hopefully it isn't too offensive :P.
>
> The motivation is that base::HashInts64 is a fairly hot function because
> it is
> called by TrackedObjects::TallyABirthIfActive (one of the hottest
> according to
> chromeos profiling). However, on a 32-bit platform using HashInts64 on
> 32-bit
> values doesn't buy us any additional entropy (*hand-wavey*) compared to
> using
> HashInts32. It is more expensive, generating an extra 4 64-bit multiplies,
> each
> results in a function call (to _allmul) on Windows-32. Oh, and
> base::HashInts64
> has many crash reports on x86, but none on x86-64. No idea why, since the
> generated code looks quite reasonable. Probably bad machines.
>
> Now, this cl has two separable parts. Changing base::HashPair to be a
> templated
> function, which should make it less brittle to type definitions. And
> changing
> the cast in base::Location to be of the "correct" size. If you want, I can
> split
> this change into two.
>

I would like that, thanks. :) More clear attribution if things go wrong.

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

[Anand Mistry (off Chromium), 2015-11-16 22:42:16]

Description was changed from

==========
Replace base::HashPair macros with a templated function.

Also, change base::Location's hash function to cast file_name to a unitptr_t
instead of uint64_t. This is expected to generate a call to base::HashInts32 on
a 32-bit platform.
==========

to

==========
Replace base::HashPair macros with a templated function.
==========

[Anand Mistry (off Chromium), 2015-11-16 22:43:19]

On 2015/11/16 19:37:33, danakj (behind on reviews) wrote:
> On Sun, Nov 15, 2015 at 6:22 PM, <mailto:amistry@chromium.org> wrote:
> 
> > Reviewers: danakj (behind on reviews)
> > CL: https://codereview.chromium.org/1445013002/
> >
> > Message:
> > I'd like to propose this change. Hopefully it isn't too offensive :P.
> >
> > The motivation is that base::HashInts64 is a fairly hot function because
> > it is
> > called by TrackedObjects::TallyABirthIfActive (one of the hottest
> > according to
> > chromeos profiling). However, on a 32-bit platform using HashInts64 on
> > 32-bit
> > values doesn't buy us any additional entropy (*hand-wavey*) compared to
> > using
> > HashInts32. It is more expensive, generating an extra 4 64-bit multiplies,
> > each
> > results in a function call (to _allmul) on Windows-32. Oh, and
> > base::HashInts64
> > has many crash reports on x86, but none on x86-64. No idea why, since the
> > generated code looks quite reasonable. Probably bad machines.
> >
> > Now, this cl has two separable parts. Changing base::HashPair to be a
> > templated
> > function, which should make it less brittle to type definitions. And
> > changing
> > the cast in base::Location to be of the "correct" size. If you want, I can
> > split
> > this change into two.
> >
> 
> I would like that, thanks. :) More clear attribution if things go wrong.

Done. I've also checked the generated code on 32 and 64-bit linux release to
make sure the condition gets folded and eliminated as expected.

> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-11-16 22:51:50]

https://codereview.chromium.org/1445013002/diff/60001/base/containers/hash_ta...
File base/containers/hash_tables.h (right):

https://codereview.chromium.org/1445013002/diff/60001/base/containers/hash_ta...
base/containers/hash_tables.h:254: if (sizeof(T1) >= sizeof(uint64_t) ||
(sizeof(T2) >= sizeof(uint64_t)))
do you mean > sizeof(uint32_t)?

[danakj, 2015-11-16 22:51:54]

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

[commit-bot: I haz the power, 2015-11-16 22:52:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445013002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445013002/60001

[commit-bot: I haz the power, 2015-11-16 22:56:38]

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

[commit-bot: I haz the power, 2015-11-16 22:56:38]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Anand Mistry (off Chromium), 2015-11-16 23:01:45]

https://codereview.chromium.org/1445013002/diff/60001/base/containers/hash_ta...
File base/containers/hash_tables.h (right):

https://codereview.chromium.org/1445013002/diff/60001/base/containers/hash_ta...
base/containers/hash_tables.h:254: if (sizeof(T1) >= sizeof(uint64_t) ||
(sizeof(T2) >= sizeof(uint64_t)))
On 2015/11/16 22:51:50, danakj (behind on reviews) wrote:
> do you mean > sizeof(uint32_t)?

Done. I don't think it matters in practice, but that's a better reflection of
the intention.

[Anand Mistry (off Chromium), 2015-11-17 00:29:01]

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

[commit-bot: I haz the power, 2015-11-17 00:30:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445013002/60002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445013002/60002

[commit-bot: I haz the power, 2015-11-17 04:20:46]

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

[commit-bot: I haz the power, 2015-11-17 04:20:47]

Dry run: This issue passed the CQ dry run.

[Anand Mistry (off Chromium), 2015-11-23 23:06:37]

Ping! Let me know if you disagree so I can drop this change.

[danakj, 2015-11-23 23:22:06]

Sorry didn't see your reply, musta fallen out of inbox. LGTM

[Anand Mistry (off Chromium), 2015-11-23 23:46:58]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2015-11-23 23:48:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445013002/60002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445013002/60002

[commit-bot: I haz the power, 2015-11-24 03:22:49]

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

[commit-bot: I haz the power, 2015-11-24 03:22:52]

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

[Anand Mistry (off Chromium), 2015-11-24 08:11:16]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2015-11-24 08:12:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445013002/60002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445013002/60002

[commit-bot: I haz the power, 2015-11-24 09:06:01]

Committed patchset #5 (id:60002)

[commit-bot: I haz the power, 2015-11-24 09:07:21]

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