Move TraceEvent.h from platform/ to wtf/

Move TraceEvent.h from platform/ to wtf/

Currently we cannot use trace events in heap/, because TraceEvent is implemented in platform/ and heap/ cannot depend on platform/.

To address the issue, this CL moves TraceEvent.h from platform/ to wtf/.

[haraken, 2014-03-21 17:10:17]

https://codereview.chromium.org/208263004/diff/20001/Source/wtf/TraceEvent.h
File Source/wtf/TraceEvent.h (right):

https://codereview.chromium.org/208263004/diff/20001/Source/wtf/TraceEvent.h#...
Source/wtf/TraceEvent.h:166: #include "platform/EventTracer.h"

Unfortunately this CL doesn't work because of this include: wtf/TraceEvent.h
needs to include platform/EventTracer.h...

- TraceEvent somehow needs to interact with platform APIs, which means that
TraceEvent depends on platform/, which means that TraceEvent cannot be put into
wtf/.

- We want to use TraceEvent in heap/, which means that TraceEvent should be put
into wtf/.

Do you have any idea to address this issue? One possible approach would be to
keep TraceEvent in platform/ and pass a function pointer to heap/ so that heap/
can use TraceEvent. But it's ugly.

[abarth-chromium, 2014-03-21 17:14:41]

Why didn't the DEPS check flag this as illegal?  As haraken says, this is
introducing a bad dependency from wtf to platform.

[haraken, 2014-03-21 17:24:08]

On 2014/03/21 17:14:41, abarth wrote:
> Why didn't the DEPS check flag this as illegal?  As haraken says, this is
> introducing a bad dependency from wtf to platform.

Sorry about the confusion. The DEPS check warned that this CL is doing a wrong
thing. Thus I'm asking what I should do in comment #1 :)

[abarth-chromium, 2014-03-21 17:27:56]

On 2014/03/21 17:24:08, haraken wrote:
> Sorry about the confusion. The DEPS check warned that this CL is doing a wrong
> thing. Thus I'm asking what I should do in comment #1 :)

Ah, I was confused.  I didn't realize that you were the author of the CL.  :)

If you really want heap to depend on blink::Platform, we should probably just
move heap into blink_platform.dll.  For example, you could move the code to
Source/platform/heap.

[haraken, 2014-03-21 17:34:04]

On 2014/03/21 17:27:56, abarth wrote:
> On 2014/03/21 17:24:08, haraken wrote:
> > Sorry about the confusion. The DEPS check warned that this CL is doing a
wrong
> > thing. Thus I'm asking what I should do in comment #1 :)
> 
> Ah, I was confused.  I didn't realize that you were the author of the CL.  :)
> 
> If you really want heap to depend on blink::Platform, we should probably just
> move heap into blink_platform.dll.  For example, you could move the code to
> Source/platform/heap.

mads@: What do you think? (It sounds a bit overkilling to move heap/ to
platform/ just for TraceEvent, but if the move will make things easier (other
than TraceEvent), I'm sure we should do that.)

[Mads Ager (chromium), 2014-03-24 07:11:08]

Hmm, good question. Am I right to assume that platform/ is intended for
code that is shared between core/ and modules/? Core and modules can depend
on platform, but not the other way around? Heap is definitely a platform
layer in that respect as well. The one additional thing for heap is that
public depends on heap because the Blink API needs to be able to keep
things alive with Persistent handles (WebPrivatePtr). Concepts in platform/
will definitely have to depend on heap/ as soon as life-time is involved
(Supplementable is one example). I think it makes sense to think of heap as
part of a platform layer, either by allowing the cyclic dependency or by
moving them together. Below I have included my (perhaps naive view of the
dependencies). We can move heap and platform together if that would be
preferable to the cyclic dependencies. In the end it boils down to the same
thing. The other alternative would be to take the basic things such as
logging and tracing and move them out of platform and into their own base
layer that (like wtf) everyone can depend on, but it might be hard to
separate them out in a meaningful way?

Cheers,    -- Mads

PS: It seems like the dependencies do not follow my understanding at this
point. For example, platform/TaskSynchronizer.h depends on platform, heap,
and modules/webdatabase. In my understanding that means that
TaskSynchronizer should be part of modules/webdatabase to avoid the
dependency from platform/ to modules/?

[image: Inline image 1]


On Fri, Mar 21, 2014 at 6:34 PM, <haraken@chromium.org> wrote:

> On 2014/03/21 17:27:56, abarth wrote:
>
>> On 2014/03/21 17:24:08, haraken wrote:
>> > Sorry about the confusion. The DEPS check warned that this CL is doing a
>>
> wrong
>
>> > thing. Thus I'm asking what I should do in comment #1 :)
>>
>
>  Ah, I was confused.  I didn't realize that you were the author of the CL.
>>  :)
>>
>
>  If you really want heap to depend on blink::Platform, we should probably
>> just
>> move heap into blink_platform.dll.  For example, you could move the code
>> to
>> Source/platform/heap.
>>
>
> mads@: What do you think? (It sounds a bit overkilling to move heap/ to
> platform/ just for TraceEvent, but if the move will make things easier
> (other
> than TraceEvent), I'm sure we should do that.)
>
>
> https://codereview.chromium.org/208263004/
>

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

[Mads Ager (chromium), 2014-03-24 07:14:01]

And the drawing is here:
https://docs.google.com/drawings/d/19jLEmaUb6aWQHjFlf4o46uNmCW0UtGakl3_BRqaVS...

[haraken, 2014-03-24 07:16:13]

+jochen who knows a lot about the layering of Blink.

[tkent, 2014-03-24 07:27:49]

On 2014/03/24 07:11:08, Mads Ager (chromium) wrote:
> Am I right to assume that platform/ is intended for
> code that is shared between core/ and modules/?

Platform/ is just a layer between wtf/ and others.  Web/, devtools/, bindings/
may depend on platform/.

> Core and modules can depend
> on platform, but not the other way around? 

Right.

IMO, moving heap/ into platform/ sounds reasonable. We shouldn't move TraceEvent
to wtf/.

[Mads Ager (chromium), 2014-03-24 07:31:24]

On 2014/03/24 07:27:49, tkent wrote:
> On 2014/03/24 07:11:08, Mads Ager (chromium) wrote:
> > Am I right to assume that platform/ is intended for
> > code that is shared between core/ and modules/?
> 
> Platform/ is just a layer between wtf/ and others.  Web/, devtools/, bindings/
> may depend on platform/.
> 
> > Core and modules can depend
> > on platform, but not the other way around? 
> 
> Right.
> 
> IMO, moving heap/ into platform/ sounds reasonable. We shouldn't move
TraceEvent
> to wtf/.

I agree that tracing shouldn't be in wtf. I was considering if logging and
tracing should be its own new module that is small and self-contained so we can
allow everyone to depend on that (in the same way that everyone can depend on
wtf)?

[haraken, 2014-03-24 07:34:27]

On 2014/03/24 07:31:24, Mads Ager (chromium) wrote:
> On 2014/03/24 07:27:49, tkent wrote:
> > On 2014/03/24 07:11:08, Mads Ager (chromium) wrote:
> > > Am I right to assume that platform/ is intended for
> > > code that is shared between core/ and modules/?
> > 
> > Platform/ is just a layer between wtf/ and others.  Web/, devtools/,
bindings/
> > may depend on platform/.
> > 
> > > Core and modules can depend
> > > on platform, but not the other way around? 
> > 
> > Right.
> > 
> > IMO, moving heap/ into platform/ sounds reasonable. We shouldn't move
> TraceEvent
> > to wtf/.
> 
> I agree that tracing shouldn't be in wtf. I was considering if logging and
> tracing should be its own new module that is small and self-contained so we
can
> allow everyone to depend on that (in the same way that everyone can depend on
> wtf)?

This is also tricky, since tracing needs to interact with the platform/ layer.
The core of the tracing infrastructure is implemented in the Chromium side, and
the platform/ layer does the interaction.

[abarth-chromium, 2014-03-24 16:25:42]

On 2014/03/24 07:11:08, Mads Ager (chromium) wrote:
> Core and modules can depend on platform, but not the other way around?

Correct.

> Heap is definitely a platform layer in that respect as well.

There isn't really such thing as "a platform layer".  The platform directly just
contains all the code that gets linked into blink_platform.dll.  Currently, the
heap code gets linked into a separate DLL, but we could change our minds and
link it into blink_platform.dll if we need to add a dependency on other code in
blink_platform.dll.

> The one additional thing for heap is that
> public depends on heap because the Blink API needs to be able to keep
> things alive with Persistent handles (WebPrivatePtr).

That's fine.  The public API for blink_platform.dll is contained in
public/platform.  The other public headers are allowed to depend on
public/platform and hence blink_platform.dll.

> Concepts in platform/
> will definitely have to depend on heap/ as soon as life-time is involved
> (Supplementable is one example). I think it makes sense to think of heap as
> part of a platform layer, either by allowing the cyclic dependency or by
> moving them together.

Cyclic dependencies are impossible.  If we have cyclic dependencies, the product
will not run on Windows.

> Below I have included my (perhaps naive view of the
> dependencies). We can move heap and platform together if that would be
> preferable to the cyclic dependencies.

They're certainly preferable to cyclic dependencies because we can't have cyclic
dependencies.

> In the end it boils down to the same thing.

The build products are different, so it's not really the same thing. 
Fortunately, we recently merged blink_origin.dll back into blink_platform.dll,
which makes this decision much easier.  Previously, blink_origin.dll was below
blink_platform.dll because in the production configuration on Windows,
blink_origin.dll was linked into the binaries for both the parent and child
processes.  Now it's only linked into the child binary, which means we can merge
it back into blink_platform.dll without bloating the parent binary.  Given that
SecurityOrigin is going to depend on heap, we no longer have a requirement for
code below blink_platform.dll depending on heap, which means we can move heap
into blink_platform.dll.

> The other alternative would be to take the basic things such as
> logging and tracing and move them out of platform and into their own base
> layer that (like wtf) everyone can depend on, but it might be hard to
> separate them out in a meaningful way?

The problem is tracing depends on the blink::Platform interface.  One of the
reasons that blink_platform.dll is separate from wtf.dll is that
blink_platform.dll is allowed to depend on blink::Platform but wtf.dll is not.

> PS: It seems like the dependencies do not follow my understanding at this
> point. For example, platform/TaskSynchronizer.h depends on platform, heap,
> and modules/webdatabase.

That's very strange.  It looks like that file isn't compiled.  If it were, it
would fail to link.

> In my understanding that means that
> TaskSynchronizer should be part of modules/webdatabase to avoid the
> dependency from platform/ to modules/?

It looks like that file should just be deleted because it's not used.  I'll work
up a CL that does that.

[abarth-chromium, 2014-03-24 16:28:05]

Removal in https://codereview.chromium.org/199793020

[Mads Ager (chromium), 2014-03-24 17:58:27]

Thanks for the explanation Adam (and for removing the files that made me
confused about whether or not cycles would work or not!) It seems that we should
make heap part of platform. We really should have trace events for garbage
collection. :-)

[abarth-chromium, 2014-03-24 18:03:26]

sgtm