cc: Adding creation location to debug BeginFrameArgs objects.

cc: Adding creation location to debug BeginFrameArgs objects.

This allows easy tracing of a BeginFrameArgs object back to the
BeginFrameSource that created it. This is very useful as we have multiple
BeginFrameSources in the system now. The Primary / Background sources being a
perfect example.

While location tracking is highly optimised (as it is used in every base::Bind
calls) it doubles the size of BeginFrameArgs objects. Hence we only enable
location tracking in debug builds. In release builds we make sure to never
create the Location objects in the first place, so no extra strings are found
in the binary. This can be checked with the strings tool.

BUG=346230
DEPS=742683002

Committed: https://crrev.com/06d1f3bfeece8b32c67bc41d4b4d7d6dcec01b32
Cr-Commit-Position: refs/heads/master@{#306326}

[mithro-old, 2014-11-19 09:59:12]

Patchset #1 (id:1) has been deleted

[mithro-old, 2014-11-19 10:02:19]

mithro@mithis.com changed reviewers:
+ brianderson@chromium.org, skyostil@chromium.org

[mithro-old, 2014-11-19 10:02:20]

Hi!

This patch depends on the clean up in https://codereview.chromium.org/742683002/

It adds the ability to figure out where a BeginFrameArgs was created in debug
builds. It has made figuring out if this BeginFrameArgs currently in the retro
frame queue has come from the background source or foreground source easy.

It should not affect the performance or codesize of release builds too (at a
slight debug build speed decrease).

Tim 'mithro' Ansell

[danakj, 2014-11-19 15:46:00]

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

[danakj, 2014-11-19 15:46:00]

https://codereview.chromium.org/735723005/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/735723005/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:333:
CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, time_ticks));
Is there any need to pass a correct FROM_HERE in the ForTesting version? Can it
just insert the FROM_HERE in there to save tests from having to worry about it?

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:21: /**
Use c++ style comments

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:53: // The location argument should **always** be
BEGINFRAME_FROM_HERE macro.
The **'s seem excessive?

[brianderson, 2014-11-19 20:43:30]

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.cc:55: delete location;
Would it be better to use a scoped_ptr instead of manually deleting?

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:83: #ifndef NDEBUG
Do we have any kind of policy against differing struct layouts between Release
and Debug builds? I tried to find it in the style guides, but I couldn't find
anything about the topic.

I know some projects have that policy to avoid hard to debug errors in case devs
mix and match Release/Debug libraries. I don't know of any chrome devs that do
that though.

[danakj, 2014-11-19 20:45:45]

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:83: #ifndef NDEBUG
On 2014/11/19 20:43:30, brianderson wrote:
> Do we have any kind of policy against differing struct layouts between Release
> and Debug builds? I tried to find it in the style guides, but I couldn't find
> anything about the topic.
> 
> I know some projects have that policy to avoid hard to debug errors in case
devs
> mix and match Release/Debug libraries. I don't know of any chrome devs that do
> that though.

We've done this before, though we tend to do it by DCHECK_IS_ON now. This is
fine to do.

[mithro-old, 2014-11-20 04:30:13]

Currently investigating if there is a better way.

I added a couple of typedefs which might make this a bit cleaner.

Tim 'mithro' Ansell

https://codereview.chromium.org/735723005/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/735723005/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:333:
CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, time_ticks));
On 2014/11/19 15:46:00, danakj wrote:
> Is there any need to pass a correct FROM_HERE in the ForTesting version? Can
it
> just insert the FROM_HERE in there to save tests from having to worry about
it?

It actually is *super* useful in tests when things break. It lets you know
exactly which line created that random BeginFrameArgs that doesn't make any
sense.

Wish C++ provided a better way to do this rather than MACROs :(

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.cc:55: delete location;
On 2014/11/19 20:43:29, brianderson wrote:
> Would it be better to use a scoped_ptr instead of manually deleting?

I tried to use a scope pointer but C++ is not smart enough to understand that
the following should work;
 void A(scoped_ptr<blah> a) {
   B(a);
 }

Instead you have to do;
 void A(scoped_ptr<blah> a) {
   B(a.Pass());
 }

Which means that things need to understand that the first argument is a scoped
pointer or not.

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:83: #ifndef NDEBUG
On 2014/11/19 20:43:30, brianderson wrote:
> Do we have any kind of policy against differing struct layouts between Release
> and Debug builds? I tried to find it in the style guides, but I couldn't find
> anything about the topic.
> 
> I know some projects have that policy to avoid hard to debug errors in case
devs
> mix and match Release/Debug libraries. I don't know of any chrome devs that do
> that though.

I would normally 100% agree that it is a bad idea to have these types be
different in Debug/Release builds. However it does *double* the space used by a
BeginFrameArgs object and that would means the object is starting to get to the
size were pass-by-value isn't the best idea anymore .

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:83: #ifndef NDEBUG
On 2014/11/19 20:45:45, danakj wrote:
> On 2014/11/19 20:43:30, brianderson wrote:
> > Do we have any kind of policy against differing struct layouts between
Release
> > and Debug builds? I tried to find it in the style guides, but I couldn't
find
> > anything about the topic.
> > 
> > I know some projects have that policy to avoid hard to debug errors in case
> devs
> > mix and match Release/Debug libraries. I don't know of any chrome devs that
do
> > that though.
> 
> We've done this before, though we tend to do it by DCHECK_IS_ON now. This is
> fine to do.

What is the difference between DCHECK_IS_ON and NDEBUG?

[Sami, 2014-11-20 11:36:14]

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:34: #define BEGINFRAME_FROM_HERE (new FROM_HERE)
Do we need to allocate FROM_HEREs on the heap? They can usually be passed around
by value. I don't think creating a dummy location in release mode would add all
that much code if you consider that there aren't that many places that create
BeginFrames in non-test code.

Which compilers did you check that the strings cannot be pruned? At least gcc
4.8.2 with -flto was able to drop the strings even when they were passed between
compilation units.

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:55: tracked_objects::Location created_by;
bikeshed: created_from?

[picksi1, 2014-11-20 14:04:19]

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

[picksi1, 2014-11-20 14:04:20]

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.cc:13: switch (type) {
should this be a const function?

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.cc:24: return "???";
If this return value is Logged somewhere (in the trace events, I guess) it might
be helpful to explicitly say "UNKNOWN BEGIN FRAME TYPE" - it would also be
really nice to add the numeric value of the unknown type...

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:47:
(source).TestOnBeginFrame(new_args);                              \
ASSERT_FALSE(old_args == new_args) ?

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:168: 
nit: This looks like it would all fit on the same line?

[danakj, 2014-11-20 16:02:20]

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.cc:55: delete location;
On 2014/11/20 04:30:13, mithro wrote:
> On 2014/11/19 20:43:29, brianderson wrote:
> > Would it be better to use a scoped_ptr instead of manually deleting?
> 
> I tried to use a scope pointer but C++ is not smart enough to understand that
> the following should work;
>  void A(scoped_ptr<blah> a) {
>    B(a);
>  }

It's not a matter of smarts. scoped_ptr is move-only.

> Instead you have to do;
>  void A(scoped_ptr<blah> a) {
>    B(a.Pass());
>  }
> 
> Which means that things need to understand that the first argument is a scoped
> pointer or not.
> 

Why is that a problem here?

#define BEGINFRAME_FROM_HERE make_scoped_ptr(new FROM_HERE)

BeginFrameArgs::Create(scoped_ptr<t_o::Location> location, ..._) {
  ...
  args.created_by = *location;
}

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:83: #ifndef NDEBUG
On 2014/11/20 04:30:13, mithro wrote:
> On 2014/11/19 20:45:45, danakj wrote:
> > On 2014/11/19 20:43:30, brianderson wrote:
> > > Do we have any kind of policy against differing struct layouts between
> Release
> > > and Debug builds? I tried to find it in the style guides, but I couldn't
> find
> > > anything about the topic.
> > > 
> > > I know some projects have that policy to avoid hard to debug errors in
case
> > devs
> > > mix and match Release/Debug libraries. I don't know of any chrome devs
that
> do
> > > that though.
> > 
> > We've done this before, though we tend to do it by DCHECK_IS_ON now. This is
> > fine to do.
> 
> What is the difference between DCHECK_IS_ON and NDEBUG?

You can build a release build with dchecks on via a gyp define. Our bots do
this.

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:34: #define BEGINFRAME_FROM_HERE (new FROM_HERE)
On 2014/11/20 11:36:13, Sami wrote:
> Do we need to allocate FROM_HEREs on the heap? They can usually be passed
around
> by value. I don't think creating a dummy location in release mode would add
all
> that much code if you consider that there aren't that many places that create
> BeginFrames in non-test code.
> 
> Which compilers did you check that the strings cannot be pruned? At least gcc
> 4.8.2 with -flto was able to drop the strings even when they were passed
between
> compilation units.

I thought that too, but then you can't avoid allocating one in release builds.

[danakj, 2014-11-20 16:03:02]

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.cc:24: return "???";
On 2014/11/20 14:04:20, picksi1 wrote:
> If this return value is Logged somewhere (in the trace events, I guess) it
might
> be helpful to explicitly say "UNKNOWN BEGIN FRAME TYPE" - it would also be
> really nice to add the numeric value of the unknown type...

The NOTREACHED implies we never get to this line. We don't need to do anything
sane here.

[danakj, 2014-11-20 16:03:29]

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:168: 
On 2014/11/20 14:04:20, picksi1 wrote:
> nit: This looks like it would all fit on the same line?

Whatever git cl format does is right % filing a bug against it.

[Sami, 2014-11-20 16:26:39]

On 2014/11/20 16:02:20, danakj wrote:
> > Do we need to allocate FROM_HEREs on the heap? They can usually be passed
> around
> > by value. I don't think creating a dummy location in release mode would add
> all
> > that much code if you consider that there aren't that many places that
create
> > BeginFrames in non-test code.
> > 
> > Which compilers did you check that the strings cannot be pruned? At least
gcc
> > 4.8.2 with -flto was able to drop the strings even when they were passed
> between
> > compilation units.
> 
> I thought that too, but then you can't avoid allocating one in release builds.

This feels like a micro-optimization. We create ~one BeginFrame per frame, but
FROM_HERE is already used hundreds of times per frame in release builds.

[danakj, 2014-11-20 16:28:57]

On Thu, Nov 20, 2014 at 11:26 AM, <skyostil@chromium.org> wrote:

> On 2014/11/20 16:02:20, danakj wrote:
>
>> > Do we need to allocate FROM_HEREs on the heap? They can usually be
>> passed
>> around
>> > by value. I don't think creating a dummy location in release mode would
>> add
>> all
>> > that much code if you consider that there aren't that many places that
>>
> create
>
>> > BeginFrames in non-test code.
>> >
>> > Which compilers did you check that the strings cannot be pruned? At
>> least
>>
> gcc
>
>> > 4.8.2 with -flto was able to drop the strings even when they were passed
>> between
>> > compilation units.
>>
>
>  I thought that too, but then you can't avoid allocating one in release
>> builds.
>>
>
> This feels like a micro-optimization. We create ~one BeginFrame per frame,
> but
> FROM_HERE is already used hundreds of times per frame in release builds.
>

That is a good point. Then we could just pass a const& to the Create
method, and possibly ignore it depending on build.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-11-21 03:17:04]

Hi Simon,

It appears you didn't see that this patch was dependent on
https://codereview.chromium.org/742683002 and hence the things you commented on
are fixed over there.

Sadly Rietveld/trybots doesn't really give a good way of making this work
properly. To work around this I upload both versions with the full dependencies
(so I can run on the try bots) and a version with just the changes in this
patch.

Tim

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.cc:13: switch (type) {
On 2014/11/20 14:04:20, picksi1 wrote:
> should this be a const function?

Static functions can't be const.

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.cc:24: return "???";
On 2014/11/20 14:04:20, picksi1 wrote:
> If this return value is Logged somewhere (in the trace events, I guess) it
might
> be helpful to explicitly say "UNKNOWN BEGIN FRAME TYPE" - it would also be
> really nice to add the numeric value of the unknown type...

This follows the approach in cc/scheduler/scheduler_state_machine.cc

This function needs to return a "const char*" so there is no way to include the
numeric value without leaking memory.

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.cc:24: return "???";
On 2014/11/20 16:03:02, danakj wrote:
> On 2014/11/20 14:04:20, picksi1 wrote:
> > If this return value is Logged somewhere (in the trace events, I guess) it
> might
> > be helpful to explicitly say "UNKNOWN BEGIN FRAME TYPE" - it would also be
> > really nice to add the numeric value of the unknown type...
> 
> The NOTREACHED implies we never get to this line. We don't need to do anything
> sane here.

NOTREACHED() doesn't fail where DCHECK is not enabled, so we still need to
return something.

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:47:
(source).TestOnBeginFrame(new_args);                              \
On 2014/11/20 14:04:20, picksi1 wrote:
> ASSERT_FALSE(old_args == new_args) ?

Done.

[mithro-old, 2014-11-21 05:57:47]

> This feels like a micro-optimization. We create ~one BeginFrame per frame, but
> FROM_HERE is already used hundreds of times per frame in release builds.

I totally agree that premature optimisation is generally evil and that
micro-optimization is often just a form of premature optimisation. Thanks for
being vigilant and making me explain my reasoning here. While writing down this
reasoning took a long time, it did help clarify my thinking.

I'm wondering how to document this information in the source code. Any
suggestions on what bits should end up in it?

There are two different optimizations going on here;

 * Not having Location objects on BeginFrameArgs in release builds.

    Firstly, some information;
      - BeginFrameArgs are mainly used as a "pass-by-value" object. 
      - BeginFrameArgs are currently 4 * int64 objects big, thus 32 bytes big.
      - Atracked_object::Location object is 4 * ptr + 1 x int long, thus ~40
bytes on 64bit platform.

    This means, adding a tracked_object::Location object to BeginFrameArgs more
than doubles the size of the BeginFrameArgs object. 

    The current "rule of thumb" is that on a modern computer 64 bytes[3] is
roughly the boundary where things should no longer be pass-by-value objects[1].
Thus, I'm pretty sure we don't want the BeginFrameArgs to have a
tracked_object::Location in release builds.

    Another thing to note is that while initial BeginFrameArgs values are only
created once per frame (hopefully inside a BeginFrameSource), they are actually
passed around quite a bit (and probably even more in the future).

 [1]: This is simplifying a *super* complicated issue. Firstly, this "rule of
thumb" has nothing to do with "pass by register"[2].  This is referring to the
point at which the speed of having to copy the data meets the cost of having to
dereference a pointer (and the associated issues that it causes for pipelining /
processor caches / etc). The exact value can vary widely due to many factors and
should only be used as a very ***rough*** guide. Your milage may vary, warrant
void where allowed by law, etc...

 [2]: Modern compilers can deconstruct smaller structure (say one to three
register sizes) into registers and then pass via register. This is much easier
to do with pass-by value and is significantly faster than passing by the stack.
On systems like AARCH64 and AMD64 the plentiful registers mean that even
structures which are 3 or 4 pointers in size can have this happen if they are
the only argument to a function.

 [3]: It is crazy that this amount is roughly ~64 bytes for a modern 64bit
processor from the last 10ish years (and this seems to be even true for mobile
processors in the last 4-5ish years too!). Some microbenchmarks show that on the
current generation intel (desktop) processors the rule should be 256 or even 512
bytes!

----

 * Not having extras strings required by Location objects appear in Release
builds.

    The reason that I'm concerned about these strings is that on an Android
system, there are roughly 8 locations where BeginFrameArgs are created that are
compiled into a release build of Chrome (despite only ~3 of these locations
normally being used). At roughly ~200 characters (thus bytes) required for each
location information this means these strings roughly expand the binary size by
1.6kb! 

    As far as I can tell, these strings are only stripped from the binary under
very strict circumstances. It definitely requires link time optimisation to even
have a chance to identify that they can be removed.

    While at first it seems like it should be relatively easy to "optimized out"
these values. If you think about it from a compiler point of view it is actually
harder than it seems. What the compiler needs to do is;

      a) Identify that the tracked_object::Location object has no side-effects
through just being allocated / deallocated (IE is a POD type).
      b) Identify that the BeginFrameArgs::Create function never uses the passed
in tracked_object::Location object.
      c) Identify that the call path from where the tracked_object::Location
object is created down to the BeginFrameArgs::Create function is also not
affected by the location object. This could be across compile units, hence why
link time optimization is needed.
      d) Then it needs to realize that the tracked_object::Location object thus
never need to be created.
      e) Then it needs to realize that the const char strings are no longer
referenced / used in any way.

    Only *then* can it decide to discard the string values. It is pretty amazing
that a modern compiler even have a chance of doing all this! The problem is that
it only requires one of these steps to fall part in some way for the strings to
end up in the final binary.

----

As you pointed out creation of tracked_object::Location objects is done about a
bazillion times every frame (every time Bind is called). Location object just
consists of a couple of const pointers, they are *super fast* to create and pass
around (specially being under the 64 byte size too). The creation speed was not
an influence on this patch.

Hope this all makes sense!

Tim 'mithro' Ansell

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.cc:55: delete location;
On 2014/11/20 16:02:19, danakj wrote:
> On 2014/11/20 04:30:13, mithro wrote:
> > On 2014/11/19 20:43:29, brianderson wrote:
> > > Would it be better to use a scoped_ptr instead of manually deleting?
> > 
> > I tried to use a scope pointer but C++ is not smart enough to understand
that
> > the following should work;
> >  void A(scoped_ptr<blah> a) {
> >    B(a);
> >  }
> 
> It's not a matter of smarts. scoped_ptr is move-only.

Yes, scoped_ptr is move-only. The compiler should be able to see that in the
above case B(a) can be transformed into a move operation.

As the B() call is the last thing inside A() the argument "a" will be destroyed
as soon as B() returns. Hence it is perfectly safe to move argument "a" into
B().

> > Instead you have to do;
> >  void A(scoped_ptr<blah> a) {
> >    B(a.Pass());
> >  }
> > 
> > Which means that things need to understand that the first argument is a
scoped
> > pointer or not.
> > 
> 
> Why is that a problem here?
> 
> #define BEGINFRAME_FROM_HERE make_scoped_ptr(new FROM_HERE)
> 
> BeginFrameArgs::Create(scoped_ptr<t_o::Location> location, ..._) {
>   ...
>   args.created_by = *location;
> }

As scoped_ptr is move-only, using it means the first argument is no longer an
opaque type that callers don't need to understand.

This means that we can't change the type behind the callers back, IE in release
builds we can't use a void* versus an actual object in Debug builds.

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:34: #define BEGINFRAME_FROM_HERE (new FROM_HERE)
On 2014/11/20 11:36:13, Sami wrote:
> Do we need to allocate FROM_HEREs on the heap? They can usually be passed
around
> by value. I don't think creating a dummy location in release mode would add
all
> that much code if you consider that there aren't that many places that create
> BeginFrames in non-test code.

Actually, with the typedef approach I just added, these don't have to be heap
allocated anymore. See the updated change.

> Which compilers did you check that the strings cannot be pruned? At least gcc
> 4.8.2 with -flto was able to drop the strings even when they were passed
between
> compilation units.

I'm using clang shipping with chrome and the binary definitely continued to
contain the strings when I checked with "strings".

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:34: #define BEGINFRAME_FROM_HERE (new FROM_HERE)
On 2014/11/20 16:02:20, danakj wrote:
> On 2014/11/20 11:36:13, Sami wrote:
> > Do we need to allocate FROM_HEREs on the heap? They can usually be passed
> around
> > by value. I don't think creating a dummy location in release mode would add
> all
> > that much code if you consider that there aren't that many places that
create
> > BeginFrames in non-test code.
> > 
> > Which compilers did you check that the strings cannot be pruned? At least
gcc
> > 4.8.2 with -flto was able to drop the strings even when they were passed
> between
> > compilation units.
> 
> I thought that too, but then you can't avoid allocating one in release builds.

I originally thought that too but with the typedef based solution it does work.

https://codereview.chromium.org/735723005/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:55: tracked_objects::Location created_by;
On 2014/11/20 11:36:14, Sami wrote:
> bikeshed: created_from?

Done.

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/735723005/diff/120001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:168: 
On 2014/11/20 16:03:29, danakj wrote:
> On 2014/11/20 14:04:20, picksi1 wrote:
> > nit: This looks like it would all fit on the same line?
> 
> Whatever git cl format does is right % filing a bug against it.

Yeah, this is what git cl format is producing.

[mithro-old, 2014-11-21 06:02:46]

Patchset #7 (id:140001) has been deleted

[mithro-old, 2014-11-21 07:40:15]

Patchset #9 (id:200001) has been deleted

[danakj, 2014-11-21 14:58:08]

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.cc:55: delete location;
On 2014/11/21 05:57:46, mithro wrote:
> On 2014/11/20 16:02:19, danakj wrote:
> > On 2014/11/20 04:30:13, mithro wrote:
> > > On 2014/11/19 20:43:29, brianderson wrote:
> > > > Would it be better to use a scoped_ptr instead of manually deleting?
> > > 
> > > I tried to use a scope pointer but C++ is not smart enough to understand
> that
> > > the following should work;
> > >  void A(scoped_ptr<blah> a) {
> > >    B(a);
> > >  }
> > 
> > It's not a matter of smarts. scoped_ptr is move-only.
> 
> Yes, scoped_ptr is move-only. The compiler should be able to see that in the
> above case B(a) can be transformed into a move operation.
> 
> As the B() call is the last thing inside A() the argument "a" will be
destroyed
> as soon as B() returns. Hence it is perfectly safe to move argument "a" into
> B().
> 
> > > Instead you have to do;
> > >  void A(scoped_ptr<blah> a) {
> > >    B(a.Pass());
> > >  }
> > > 
> > > Which means that things need to understand that the first argument is a
> scoped
> > > pointer or not.
> > > 
> > 
> > Why is that a problem here?
> > 
> > #define BEGINFRAME_FROM_HERE make_scoped_ptr(new FROM_HERE)
> > 
> > BeginFrameArgs::Create(scoped_ptr<t_o::Location> location, ..._) {
> >   ...
> >   args.created_by = *location;
> > }
> 
> As scoped_ptr is move-only, using it means the first argument is no longer an
> opaque type that callers don't need to understand.
> 
> This means that we can't change the type behind the callers back, IE in
release
> builds we can't use a void* versus an actual object in Debug builds.

Why do you need to change the type to a void*? It can just be an empty
scoped_ptr that you don't deref?

[danakj, 2014-11-21 15:03:43]

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.cc:55: delete location;
On 2014/11/21 14:58:07, danakj wrote:
> On 2014/11/21 05:57:46, mithro wrote:
> > On 2014/11/20 16:02:19, danakj wrote:
> > > On 2014/11/20 04:30:13, mithro wrote:
> > > > On 2014/11/19 20:43:29, brianderson wrote:
> > > > > Would it be better to use a scoped_ptr instead of manually deleting?
> > > > 
> > > > I tried to use a scope pointer but C++ is not smart enough to understand
> > that
> > > > the following should work;
> > > >  void A(scoped_ptr<blah> a) {
> > > >    B(a);
> > > >  }
> > > 
> > > It's not a matter of smarts. scoped_ptr is move-only.
> > 
> > Yes, scoped_ptr is move-only. The compiler should be able to see that in the
> > above case B(a) can be transformed into a move operation.
> > 
> > As the B() call is the last thing inside A() the argument "a" will be
> destroyed
> > as soon as B() returns. Hence it is perfectly safe to move argument "a" into
> > B().
> > 
> > > > Instead you have to do;
> > > >  void A(scoped_ptr<blah> a) {
> > > >    B(a.Pass());
> > > >  }
> > > > 
> > > > Which means that things need to understand that the first argument is a
> > scoped
> > > > pointer or not.
> > > > 
> > > 
> > > Why is that a problem here?
> > > 
> > > #define BEGINFRAME_FROM_HERE make_scoped_ptr(new FROM_HERE)
> > > 
> > > BeginFrameArgs::Create(scoped_ptr<t_o::Location> location, ..._) {
> > >   ...
> > >   args.created_by = *location;
> > > }
> > 
> > As scoped_ptr is move-only, using it means the first argument is no longer
an
> > opaque type that callers don't need to understand.
> > 
> > This means that we can't change the type behind the callers back, IE in
> release
> > builds we can't use a void* versus an actual object in Debug builds.
> 
> Why do you need to change the type to a void*? It can just be an empty
> scoped_ptr that you don't deref?

Ah, I see what you've done. I don't really see the value of typedeffing it as a
void* in release builds, it's the only place that the typedef is used, and the
code has to know to not deref it anyway. What if you did:

#if debug
#define BEGINFRAME_FROM_HERE nullptr
#else
#define BEGINFRAME_FROM_HERE (&FROM_HERE)
#endif

BeginFrameArgs BeginFrameArgs::Create(const tracked_objects::Location* location,
...) {
  ...
#if debug
  created_from = *location;
#endif
}

?

https://codereview.chromium.org/735723005/diff/180001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/735723005/diff/180001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:52: typedef const void* CreationLocation;
typedefs should be at the top of the class

https://codereview.chromium.org/735723005/diff/180001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:55: tracked_objects::Location created_from;
member variables at the bottom

https://codereview.chromium.org/735723005/diff/180001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:61: static BeginFrameArgs Create(CreationLocation
location,
pass by const& please. this is > a single primitive type

[mithro-old, 2014-11-21 16:30:55]

On 2014/11/21 15:03:43, danakj wrote:
>
https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
> File cc/output/begin_frame_args.cc (right):
> 
>
https://codereview.chromium.org/735723005/diff/40001/cc/output/begin_frame_ar...
> cc/output/begin_frame_args.cc:55: delete location;
> On 2014/11/21 14:58:07, danakj wrote:
> > On 2014/11/21 05:57:46, mithro wrote:
> > > On 2014/11/20 16:02:19, danakj wrote:
> > > > On 2014/11/20 04:30:13, mithro wrote:
> > > > > On 2014/11/19 20:43:29, brianderson wrote:
> > > > > > Would it be better to use a scoped_ptr instead of manually deleting?
> > > > > 
> > > > > I tried to use a scope pointer but C++ is not smart enough to
understand
> > > that
> > > > > the following should work;
> > > > >  void A(scoped_ptr<blah> a) {
> > > > >    B(a);
> > > > >  }
> > > > 
> > > > It's not a matter of smarts. scoped_ptr is move-only.
> > > 
> > > Yes, scoped_ptr is move-only. The compiler should be able to see that in
the
> > > above case B(a) can be transformed into a move operation.
> > > 
> > > As the B() call is the last thing inside A() the argument "a" will be
> > destroyed
> > > as soon as B() returns. Hence it is perfectly safe to move argument "a"
into
> > > B().
> > > 
> > > > > Instead you have to do;
> > > > >  void A(scoped_ptr<blah> a) {
> > > > >    B(a.Pass());
> > > > >  }
> > > > > 
> > > > > Which means that things need to understand that the first argument is
a
> > > scoped
> > > > > pointer or not.
> > > > > 
> > > > 
> > > > Why is that a problem here?
> > > > 
> > > > #define BEGINFRAME_FROM_HERE make_scoped_ptr(new FROM_HERE)
> > > > 
> > > > BeginFrameArgs::Create(scoped_ptr<t_o::Location> location, ..._) {
> > > >   ...
> > > >   args.created_by = *location;
> > > > }
> > > 
> > > As scoped_ptr is move-only, using it means the first argument is no longer
> an
> > > opaque type that callers don't need to understand.
> > > 
> > > This means that we can't change the type behind the callers back, IE in
> > release
> > > builds we can't use a void* versus an actual object in Debug builds.
> > 
> > Why do you need to change the type to a void*? It can just be an empty
> > scoped_ptr that you don't deref?

Can you call XXX.Pass() on a scoped_ptr which doesn't have a value?

The big problem with scoped_ptr objects is that it makes
CreateBeginFrameArgsForTesting and other similar functions need to understand
the actual types of the first argument.

> Ah, I see what you've done. I don't really see the value of typedeffing it as
a
> void* in release builds, it's the only place that the typedef is used, and the
> code has to know to not deref it anyway. What if you did:
> 
> #if debug
> #define BEGINFRAME_FROM_HERE nullptr
> #else
> #define BEGINFRAME_FROM_HERE (&FROM_HERE)
> #endif
> 
> BeginFrameArgs BeginFrameArgs::Create(const tracked_objects::Location*
location,
> ...) {
>   ...
> #if debug
>   created_from = *location;
> #endif
> }

XXX::Create(&FROM_HERE) complains about not being able to take an address from a
temporary object.

Tim

[danakj, 2014-11-21 16:33:30]

On Fri, Nov 21, 2014 at 11:30 AM, <mithro@mithis.com> wrote:

> On 2014/11/21 15:03:43, danakj wrote:
>
> https://codereview.chromium.org/735723005/diff/40001/cc/
> output/begin_frame_args.cc
>
>> File cc/output/begin_frame_args.cc (right):
>>
>
>
> https://codereview.chromium.org/735723005/diff/40001/cc/
> output/begin_frame_args.cc#newcode55
>
>> cc/output/begin_frame_args.cc:55: delete location;
>> On 2014/11/21 14:58:07, danakj wrote:
>> > On 2014/11/21 05:57:46, mithro wrote:
>> > > On 2014/11/20 16:02:19, danakj wrote:
>> > > > On 2014/11/20 04:30:13, mithro wrote:
>> > > > > On 2014/11/19 20:43:29, brianderson wrote:
>> > > > > > Would it be better to use a scoped_ptr instead of manually
>> deleting?
>> > > > >
>> > > > > I tried to use a scope pointer but C++ is not smart enough to
>>
> understand
>
>> > > that
>> > > > > the following should work;
>> > > > >  void A(scoped_ptr<blah> a) {
>> > > > >    B(a);
>> > > > >  }
>> > > >
>> > > > It's not a matter of smarts. scoped_ptr is move-only.
>> > >
>> > > Yes, scoped_ptr is move-only. The compiler should be able to see that
>> in
>>
> the
>
>> > > above case B(a) can be transformed into a move operation.
>> > >
>> > > As the B() call is the last thing inside A() the argument "a" will be
>> > destroyed
>> > > as soon as B() returns. Hence it is perfectly safe to move argument
>> "a"
>>
> into
>
>> > > B().
>> > >
>> > > > > Instead you have to do;
>> > > > >  void A(scoped_ptr<blah> a) {
>> > > > >    B(a.Pass());
>> > > > >  }
>> > > > >
>> > > > > Which means that things need to understand that the first
>> argument is
>>
> a
>
>> > > scoped
>> > > > > pointer or not.
>> > > > >
>> > > >
>> > > > Why is that a problem here?
>> > > >
>> > > > #define BEGINFRAME_FROM_HERE make_scoped_ptr(new FROM_HERE)
>> > > >
>> > > > BeginFrameArgs::Create(scoped_ptr<t_o::Location> location, ..._) {
>> > > >   ...
>> > > >   args.created_by = *location;
>> > > > }
>> > >
>> > > As scoped_ptr is move-only, using it means the first argument is no
>> longer
>> an
>> > > opaque type that callers don't need to understand.
>> > >
>> > > This means that we can't change the type behind the callers back, IE
>> in
>> > release
>> > > builds we can't use a void* versus an actual object in Debug builds.
>> >
>> > Why do you need to change the type to a void*? It can just be an empty
>> > scoped_ptr that you don't deref?
>>
>
> Can you call XXX.Pass() on a scoped_ptr which doesn't have a value?
>

Yes you can pass null.


>
> The big problem with scoped_ptr objects is that it makes
> CreateBeginFrameArgsForTesting and other similar functions need to
> understand
> the actual types of the first argument.


I don't understand why that is a problem though. But avoiding a new/free
seems cool, so since you can't do &FROM_HERE, what you have seems
reasonable.


>
>
>  Ah, I see what you've done. I don't really see the value of typedeffing
>> it as
>>
> a
>
>> void* in release builds, it's the only place that the typedef is used,
>> and the
>> code has to know to not deref it anyway. What if you did:
>>
>
>  #if debug
>> #define BEGINFRAME_FROM_HERE nullptr
>> #else
>> #define BEGINFRAME_FROM_HERE (&FROM_HERE)
>> #endif
>>
>
>  BeginFrameArgs BeginFrameArgs::Create(const tracked_objects::Location*
>>
> location,
>
>> ...) {
>>    ...
>> #if debug
>>    created_from = *location;
>> #endif
>> }
>>
>
> XXX::Create(&FROM_HERE) complains about not being able to take an address
> from a
> temporary object.
>

Darn, after I sent that I was like "I wonder...".

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-11-27 01:08:47]

Looks like I need a couple more LGTMs from OWNERs for areas outside cc that are
using BeginFrameArgs. I think a combination of danakj (for ui/), skyostil (for
some files under content/) and aelias (for other files under content/) should
work?

[brianderson, 2014-11-27 01:14:20]

lgtm

[Sami, 2014-11-27 10:40:54]

skyostil@chromium.org changed reviewers:
+ aelias@chromium.org

[Sami, 2014-11-27 10:42:31]

+aelias

content/browser/android/ and content/renderer/scheduler lgtm.

[danakj, 2014-11-27 15:07:29]

ui/compositor LGTM

[mithro-old, 2014-11-28 00:32:52]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-11-28 00:33:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/735723005/260001

[commit-bot: I haz the power, 2014-11-28 00:37:09]

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

[commit-bot: I haz the power, 2014-11-28 00:37:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mithro-old, 2014-11-28 00:52:10]

On 2014/11/28 00:37:10, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Looks like Sami is correct, I still need an owner for;

Missing LGTM from an OWNER for these files:
    content/browser/renderer_host/render_widget_host_view_android.cc
    content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc
    content/renderer/gpu/render_widget_compositor_unittest.cc

aelias, can you give that?

[aelias_OOO_until_Jul13, 2014-12-01 19:52:33]

aelias@chromium.org changed reviewers:
+ sievers@chromium.org

[aelias_OOO_until_Jul13, 2014-12-01 19:52:51]

I'm not OWNERS for content/renderer/gpu/, adding sievers@

[no sievers, 2014-12-01 20:21:19]

lgtm

[mithro-old, 2014-12-02 02:14:47]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-12-02 02:15:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/735723005/280001

[commit-bot: I haz the power, 2014-12-02 02:19:36]

Committed patchset #12 (id:280001)

[commit-bot: I haz the power, 2014-12-02 02:20:28]

Patchset 12 (id:??) landed as
https://crrev.com/06d1f3bfeece8b32c67bc41d4b4d7d6dcec01b32
Cr-Commit-Position: refs/heads/master@{#306326}