Avoid copying value in ComputedStyle CoW comparions (compareEqual)

Avoid copying value in ComputedStyle CoW comparions (compareEqual)

Because of the cast of the RHS, |u|, a copy would be generated, with
code and cycle bloat as the result. In some cases this can even have
prevented inlining. Particularly nasty examples:

 bool compareEqual(const Vector<LengthPoint>&, const Vector<LengthPoint>&) [210 bytes]
 bool compareEqual(const Vector<CSSPropertyID>&, const Vector<CSSPropertyID>&) [274 bytes]
 bool compareEqual(const Vector<GridTrackSize>&, const Vector<GridTrackSize>&) [182 bytes]
 bool compareEqual(const HashMap<String, GridArea>&, const HashMap<String, GridArea>&) [308 bytes]
 bool compareEqual(const TransformOperations&, const TransformOperations&) [441 bytes]
 bool compareEqual(const LengthBox&, const LengthBox&) [249 bytes]

Remove the U->T cast in compareEqual to avoid the copies. Nothing seems
to require this coercion (anymore?) This eliminates the above symbols
entirely. A total binary size reduction of >30k (x86-64; non-official) also
indicates even the simpler cases/types may have benefited.

Committed: https://crrev.com/12052640ac64720e5d634cd7831eb9c3ca749763
Cr-Commit-Position: refs/heads/master@{#426990}

[fs, 2016-10-21 18:53:07]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 18:53:57]

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

[fs, 2016-10-21 20:05:50]

fs@opera.com changed reviewers:
+ esprehn@chromium.org, pdr@chromium.org

[commit-bot: I haz the power, 2016-10-21 21:44:52]

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

[commit-bot: I haz the power, 2016-10-21 21:44:52]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-10-21 21:50:19]

lgtm, in what cases do we need the cast though? I'm not sure I understand why
we're comparing types like this?

[fs, 2016-10-22 09:53:52]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-22 09:54:03]

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

[fs, 2016-10-22 09:56:00]

Description was changed from

==========
Avoid copying value in ComputedStyle CoW comparions (compareEqual)

Because of the cast of the RHS, |u|, a copy would be generated, with
code and cycle bloat as the result. In some cases this can even have
prevented inlining. Particularly nasty examples:

 bool compareEqual(const Vector<LengthPoint>&, const Vector<LengthPoint>&) [210
bytes]
 bool compareEqual(const Vector<CSSPropertyID>&, const Vector<CSSPropertyID>&)
[274 bytes]
 bool compareEqual(const Vector<GridTrackSize>&, const Vector<GridTrackSize>&)
[182 bytes]
 bool compareEqual(const HashMap<String, GridArea>&, const HashMap<String,
GridArea>&) [308 bytes]
 bool compareEqual(const TransformOperations&, const TransformOperations&) [441
bytes]
 bool compareEqual(const LengthBox&, const LengthBox&) [249 bytes]

Add a same-type overload of compareEqual to avoid the copies. This
eliminates the above symbols entirely. A total binary size reduction of
>30k (x86-64; non-official) also indicates even the simpler cases/types
may have benefited.
==========

to

==========
Avoid copying value in ComputedStyle CoW comparions (compareEqual)

Because of the cast of the RHS, |u|, a copy would be generated, with
code and cycle bloat as the result. In some cases this can even have
prevented inlining. Particularly nasty examples:

 bool compareEqual(const Vector<LengthPoint>&, const Vector<LengthPoint>&) [210
bytes]
 bool compareEqual(const Vector<CSSPropertyID>&, const Vector<CSSPropertyID>&)
[274 bytes]
 bool compareEqual(const Vector<GridTrackSize>&, const Vector<GridTrackSize>&)
[182 bytes]
 bool compareEqual(const HashMap<String, GridArea>&, const HashMap<String,
GridArea>&) [308 bytes]
 bool compareEqual(const TransformOperations&, const TransformOperations&) [441
bytes]
 bool compareEqual(const LengthBox&, const LengthBox&) [249 bytes]

Remove the U->T cast in compareEqual to avoid the copies. Nothing seems
to require this coercion (anymore?) This eliminates the above symbols
entirely. A total binary size reduction of >30k (x86-64; non-official) also
indicates even the simpler cases/types may have benefited.
==========

[fs, 2016-10-22 09:57:25]

On 2016/10/21 at 21:50:19, esprehn wrote:
> lgtm, in what cases do we need the cast though? I'm not sure I understand why
we're comparing types like this?

It appears the type coercion isn't needed (at least the compiler didn't
complain, will see if bots do, but presumably they won't), so I changed the
patch to just drop the RHS cast.

[commit-bot: I haz the power, 2016-10-22 10:33:48]

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

[commit-bot: I haz the power, 2016-10-22 10:33:49]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[fs, 2016-10-22 10:46:04]

On 2016/10/22 at 09:57:25, fs wrote:
> On 2016/10/21 at 21:50:19, esprehn wrote:
> > lgtm, in what cases do we need the cast though? I'm not sure I understand
why we're comparing types like this?
> 
> It appears the type coercion isn't needed (at least the compiler didn't
complain, will see if bots do, but presumably they won't), so I changed the
patch to just drop the RHS cast.

Looks like removing the cast makes MSVC unhappy [1], mostly (entirely?) due to
differences in signedness.

[1]
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...

[fs, 2016-10-22 10:46:31]

Patchset #2 (id:20001) has been deleted

[fs, 2016-10-22 10:47:13]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-10-22 10:47:19]

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

[fs, 2016-10-22 10:50:00]

On 2016/10/22 at 10:46:04, fs wrote:
> On 2016/10/22 at 09:57:25, fs wrote:
> > On 2016/10/21 at 21:50:19, esprehn wrote:
> > > lgtm, in what cases do we need the cast though? I'm not sure I understand
why we're comparing types like this?
> > 
> > It appears the type coercion isn't needed (at least the compiler didn't
complain, will see if bots do, but presumably they won't), so I changed the
patch to just drop the RHS cast.
> 
> ... (entirely?) due to differences in signedness.

Now I see there was some warnings about "unsafe mix of types" (unsigned vs.
bool) too.

> [1]
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...

[commit-bot: I haz the power, 2016-10-22 10:56:03]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-22 10:58:34]

Description was changed from

==========
Avoid copying value in ComputedStyle CoW comparions (compareEqual)

Because of the cast of the RHS, |u|, a copy would be generated, with
code and cycle bloat as the result. In some cases this can even have
prevented inlining. Particularly nasty examples:

 bool compareEqual(const Vector<LengthPoint>&, const Vector<LengthPoint>&) [210
bytes]
 bool compareEqual(const Vector<CSSPropertyID>&, const Vector<CSSPropertyID>&)
[274 bytes]
 bool compareEqual(const Vector<GridTrackSize>&, const Vector<GridTrackSize>&)
[182 bytes]
 bool compareEqual(const HashMap<String, GridArea>&, const HashMap<String,
GridArea>&) [308 bytes]
 bool compareEqual(const TransformOperations&, const TransformOperations&) [441
bytes]
 bool compareEqual(const LengthBox&, const LengthBox&) [249 bytes]

Remove the U->T cast in compareEqual to avoid the copies. Nothing seems
to require this coercion (anymore?) This eliminates the above symbols
entirely. A total binary size reduction of >30k (x86-64; non-official) also
indicates even the simpler cases/types may have benefited.
==========

to

==========
Avoid copying value in ComputedStyle CoW comparions (compareEqual)

Because of the cast of the RHS, |u|, a copy would be generated, with
code and cycle bloat as the result. In some cases this can even have
prevented inlining. Particularly nasty examples:

 bool compareEqual(const Vector<LengthPoint>&, const Vector<LengthPoint>&) [210
bytes]
 bool compareEqual(const Vector<CSSPropertyID>&, const Vector<CSSPropertyID>&)
[274 bytes]
 bool compareEqual(const Vector<GridTrackSize>&, const Vector<GridTrackSize>&)
[182 bytes]
 bool compareEqual(const HashMap<String, GridArea>&, const HashMap<String,
GridArea>&) [308 bytes]
 bool compareEqual(const TransformOperations&, const TransformOperations&) [441
bytes]
 bool compareEqual(const LengthBox&, const LengthBox&) [249 bytes]

Remove the U->T cast in compareEqual to avoid the copies. Nothing seems
to require this coercion (anymore?) This eliminates the above symbols
entirely. A total binary size reduction of >30k (x86-64; non-official) also
indicates even the simpler cases/types may have benefited.

Committed: https://crrev.com/12052640ac64720e5d634cd7831eb9c3ca749763
Cr-Commit-Position: refs/heads/master@{#426990}
==========

[commit-bot: I haz the power, 2016-10-22 10:58:34]

Patchset 1 (id:??) landed as
https://crrev.com/12052640ac64720e5d634cd7831eb9c3ca749763
Cr-Commit-Position: refs/heads/master@{#426990}