Do not use extern template declarations for RectBase etc for NaCl builds

Do not use extern template declarations for RectBase etc for NaCl builds

The GPU command buffer client uses Rect/RectF from gfx.
RectBase and friends have an extern template declaration which means that
instead of inlining the x(), y(), width() and height() methods (e.g. where they
are used in the header and in gles2_implementation.cc), an external
reference is generated instead. Unlike in Chromium, the NaCl IRT build does
not build/link with the gfx/geometry implementation, which means that
these references never get defined.

So do not use the extern template declarations for NaCl.

R=bbudge@chromium.org (GPU untrusted build), danakj@chromium.org (graphics primitives OWNERS)

TEST= NaCl IRT build with no optimization
BUG= https://code.google.com/p/chromium/issues/detail?id=388035

Committed: https://crrev.com/139fb7ad98ee8af20d557ff78e52473ea36f3ab1
Cr-Commit-Position: refs/heads/master@{#291829}

[Derek Schuff, 2014-08-25 22:23:19]

dschuff@chromium.org changed reviewers:
+ bbudge@chromium.org, danakj@chromium.org
- bbudge@chromium.org (gpu untrusted build)

[Derek Schuff, 2014-08-25 22:26:16]

dschuff@chromium.org changed reviewers:
+ bradnelson@chromium.org

[Derek Schuff, 2014-08-25 22:26:16]

This went unnoticed because we've been building the IRT with O2, which caused
the calls to the Rect members to get optimized away entirely. But with O0 they
are there.
Disabling for any NaCl build seems like a big hammer; it seems like maybe we
should just do it for the IRT builds, but we don't really have a way to
differentiate, and in any case,  it's just a small optimization in linking time.

[danakj, 2014-08-25 22:35:14]

I'm a bit confused at what this CL seems to imply. Does this mean Rect::x() is
not being inlined in our other builds?

[Derek Schuff, 2014-08-25 23:24:13]

On 2014/08/25 22:35:14, danakj wrote:
> I'm a bit confused at what this CL seems to imply. Does this mean Rect::x() is
> not being inlined in our other builds?

Correct, at least for debug builds. As an example, here's one I grepped out of
my local build:
$ objdump -dr out/Debug/obj/ui/compositor/compositor.layer.o |c++filt|less
<snip>
00000000000036c0 <ui::Layer::GetTargetTransformRelativeTo(ui::Layer const*,
gfx::Transform*) const>:
<snip>
    372e:       e8 00 00 00 00          callq  3733
<ui::Layer::GetTargetTransformRelativeTo(ui::Layer const*, gfx::Transform*)
const+0x73>
                        372f: R_X86_64_PLT32    gfx::RectBase<gfx::Rect,
gfx::Point, gfx::Size, gfx::Insets, gfx::Vector2d, int>::x() const-0x4

I don't know how readable that is, but it's a disassembly (with relocations) of
compositor.layer.o showing a call to Rect::x() (I actually used nm to look for
undefined references to Rect::x() and friends, but this shows where the calls
come from). 

Normally the functions would be inlined directly or (usually in debug mode) a
copy would be emitted into each object file that contains a call to them (later,
the linker discards all but one of the copies). The extern declaration tells the
compiler to *not* emit all those duplicate copies, but instead just promises
that there will be a definition somewhere in the link, and to emit a call to it.
It makes for faster link times, sometimes a lot faster when there are e.g. many
template instantiations going on.

[danakj, 2014-08-25 23:32:54]

On 2014/08/25 23:24:13, Derek Schuff wrote:
> On 2014/08/25 22:35:14, danakj wrote:
> > I'm a bit confused at what this CL seems to imply. Does this mean Rect::x()
is
> > not being inlined in our other builds?
> 
> Correct, at least for debug builds. As an example, here's one I grepped out of
> my local build:
> $ objdump -dr out/Debug/obj/ui/compositor/compositor.layer.o |c++filt|less
> <snip>
> 00000000000036c0 <ui::Layer::GetTargetTransformRelativeTo(ui::Layer const*,
> gfx::Transform*) const>:
> <snip>
>     372e:       e8 00 00 00 00          callq  3733
> <ui::Layer::GetTargetTransformRelativeTo(ui::Layer const*, gfx::Transform*)
> const+0x73>
>                         372f: R_X86_64_PLT32    gfx::RectBase<gfx::Rect,
> gfx::Point, gfx::Size, gfx::Insets, gfx::Vector2d, int>::x() const-0x4
> 
> I don't know how readable that is, but it's a disassembly (with relocations)
of
> compositor.layer.o showing a call to Rect::x() (I actually used nm to look for
> undefined references to Rect::x() and friends, but this shows where the calls
> come from). 
> 
> Normally the functions would be inlined directly or (usually in debug mode) a
> copy would be emitted into each object file that contains a call to them
(later,
> the linker discards all but one of the copies). The extern declaration tells
the
> compiler to *not* emit all those duplicate copies, but instead just promises
> that there will be a definition somewhere in the link, and to emit a call to
it.
> It makes for faster link times, sometimes a lot faster when there are e.g.
many
> template instantiations going on.

Well that's very interesting. I will probably need to do something about that.
LGTM anyhow.

[bradn, 2014-08-26 00:05:44]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2014-08-26 00:05:45]

lgtm

[Derek Schuff, 2014-08-26 00:10:45]

The CQ bit was checked by dschuff@chromium.org

[commit-bot: I haz the power, 2014-08-26 00:11:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dschuff@chromium.org/503953002/1

[bbudge, 2014-08-26 00:13:03]

https://codereview.chromium.org/503953002/diff/1/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (left):

https://codereview.chromium.org/503953002/diff/1/ui/gfx/geometry/rect.h#oldco...
ui/gfx/geometry/rect.h:36: 
Why remove this line?

https://codereview.chromium.org/503953002/diff/1/ui/gfx/geometry/size.h
File ui/gfx/geometry/size.h (right):

https://codereview.chromium.org/503953002/diff/1/ui/gfx/geometry/size.h#newco...
ui/gfx/geometry/size.h:63: //extern template class SizeBase<Size, int>;
Did you mean for this to be #ifdef'd out?

[Derek Schuff, 2014-08-26 00:30:28]

The CQ bit was unchecked by dschuff@chromium.org

[Derek Schuff, 2014-08-26 00:32:31]

https://codereview.chromium.org/503953002/diff/1/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (left):

https://codereview.chromium.org/503953002/diff/1/ui/gfx/geometry/rect.h#oldco...
ui/gfx/geometry/rect.h:36: 
On 2014/08/26 00:13:03, bbudge wrote:
> Why remove this line?

leftover from experimental edits,fixed.

https://codereview.chromium.org/503953002/diff/1/ui/gfx/geometry/size.h
File ui/gfx/geometry/size.h (right):

https://codereview.chromium.org/503953002/diff/1/ui/gfx/geometry/size.h#newco...
ui/gfx/geometry/size.h:63: //extern template class SizeBase<Size, int>;
On 2014/08/26 00:13:03, bbudge wrote:
> Did you mean for this to be #ifdef'd out?

yes, fixed.

[Derek Schuff, 2014-08-26 00:32:35]

The CQ bit was checked by dschuff@chromium.org

[commit-bot: I haz the power, 2014-08-26 00:34:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dschuff@chromium.org/503953002/20001

[bbudge, 2014-08-26 00:35:30]

lgtm

[commit-bot: I haz the power, 2014-08-26 03:40:44]

Committed patchset #2 (20001) as 3b2bf9c92ea7791f2670e775460bc8e7db5d65aa

[commit-bot: I haz the power, 2014-09-10 02:40:02]

Patchset 2 (id:??) landed as
https://crrev.com/139fb7ad98ee8af20d557ff78e52473ea36f3ab1
Cr-Commit-Position: refs/heads/master@{#291829}

[Nico, 2014-10-01 22:20:30]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-10-01 22:20:30]

Why don't you link to gfx/geometry if you use methods from there?

[danakj, 2014-10-06 18:30:20]

On 2014/10/01 22:20:30, Nico (away until Oct 27) wrote:
> Why don't you link to gfx/geometry if you use methods from there?

Great question. dschuff@ can you answer?

[danakj, 2014-10-06 18:33:27]

On 2014/10/06 18:30:20, danakj wrote:
> On 2014/10/01 22:20:30, Nico (away until Oct 27) wrote:
> > Why don't you link to gfx/geometry if you use methods from there?
> 
> Great question. dschuff@ can you answer?

Filed https://code.google.com/p/chromium/issues/detail?id=420715 about this
also.