[Proposal] Allow using base/trace_event from Blink

[Dan Sinclair]

With the merge complete, I'd like to propose removing the tracing code in Blink and using base/trace_event directly.

This means we can get rid of the copied third_party/Webkit/Source/platform/TraceEvent.h which often lags behind base/trace_event (see recent CLs to add flow v2 support [1]) and removes the implementation (Source/platform/EventTracer*, Source/public/platform/WebConvertableToTraceFormat* and bits of code in content/child/blink_platform_impl*).

As features and additions are added to tracing they will become available to Blink without jumping through hoops.

Thoughts, objections?

Thanks,

dan

[1] https://codereview.chromium.org/1359973003/

[Primiano Tucci]

Just to add one extra use case, in memory-infra tracing one of the big problems that we still have open is estimating the memory used by trace events itself (to back-compensate their overhead, crbug.com/495628). I managed to solve that in chromium, I never solved into blink tracing as that would have meant replicating lot of the insane estimation logic. Being able to use base/trace_event would make life much easier (and maintainable) for tracing.

I'd really like to hear opinions on this proposal.

[Chris Harrelson]

Sounds great to me. I can't think of any downside.

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

[Kenneth Russell]

Sounds good to me too. I recall there being problems changing the
number of bits in one of the data structures on the Chromium side due
to this split.

Reading the commit log for TraceEvent.h it sounds like there's
Blink-specific functionality in it. Is that accurate and would that
cause problems?

[Elliott Sprehn]

What does this end up looking like? I don't want to bring in lots of other base/ or std::string related stuff yet. Can we wrap this inside a platform or WTF abstraction? Is this just using macros?

[Dana Jansens]

On Thu, Sep 24, 2015 at 2:42 PM, Elliott Sprehn <esp...@chromium.org> wrote:

What does this end up looking like? I don't want to bring in lots of other base/ or std::string related stuff yet. Can we wrap this inside a platform or WTF abstraction? Is this just using macros?

When do you think Blink should start using base things?

[Dan Sinclair]

Because of the way the macros were written there was platform specific code (the new header blink doesn't have yet makes this a bit better but hasn't been ported to blink). But, I believe all the blink specific stuff is to call EventTracer which calls down to the platform.

Dan

[Elliott Sprehn]

On Thu, Sep 24, 2015 at 2:46 PM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, Sep 24, 2015 at 2:42 PM, Elliott Sprehn <esp...@chromium.org> wrote:

What does this end up looking like? I don't want to bring in lots of other base/ or std::string related stuff yet. Can we wrap this inside a platform or WTF abstraction? Is this just using macros?

When do you think Blink should start using base things?

Blink should not be using std:: strings or containers or the associated base libraries, the other base libraries we should discuss separately. For the latter case the answer is "not yet".

- E

[Dana Jansens]

It's fine to not use std::string and we can even PRESUBMIT enforce that if you're worried that people will start using std::string or base string things to prevent that. But that's really different than a blanket ban on base, which affects any other useful things, such as tracing.

If you're looking for discussion about individual things in base (with what granularity) isn't this such a discussion? I'm just not sure what we are waiting for now that we're merged, what is going to change "not yet" into "yet" that we can watch and work on?

[Elliott Sprehn]

On Thu, Sep 24, 2015 at 2:59 PM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, Sep 24, 2015 at 2:53 PM, Elliott Sprehn <esp...@chromium.org> wrote:

On Thu, Sep 24, 2015 at 2:46 PM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, Sep 24, 2015 at 2:42 PM, Elliott Sprehn <esp...@chromium.org> wrote:

What does this end up looking like? I don't want to bring in lots of other base/ or std::string related stuff yet. Can we wrap this inside a platform or WTF abstraction? Is this just using macros?

When do you think Blink should start using base things?

Blink should not be using std:: strings or containers or the associated base libraries, the other base libraries we should discuss separately. For the latter case the answer is "not yet".

It's fine to not use std::string and we can even PRESUBMIT enforce that if you're worried that people will start using std::string or base string things to prevent that. But that's really different than a blanket ban on base, which affects any other useful things, such as tracing.

If you're looking for discussion about individual things in base (with what granularity) isn't this such a discussion?

Yes, that's what I said in my first email.

 

I'm just not sure what we are waiting for now that we're merged, what is going to change "not yet" into "yet" that we can watch and work on?

That's exactly the question I asked, what dependencies does tracing bring in, what does the code look like after this change? It's hard to evaluate if we should do this without a patch that converts some trace points to the "new world".

- E

[Elliott Sprehn]

Specifically the question here is if we want some abstraction in platform/ or wtf/ that wraps the base/ stuff, or if we want to just use base's tracing libraries directly across all of blink. I totally support breaking down some walls. :)

[Dan Sinclair]

We have the platform wrapper now which is what causes the headache.  Unless someone beats me to it I'll try to figure out what tracing pulls in on Monday.

Dan

[Kentaro Hara]

I support adding a dependency from wtf/ to base/.

About four months ago, we did a ton of discussions in blink-dev@ and BlinkOn4 about a Blink architecture after the repository merge. See this thread and this thread. Then I summarized opinions people have agreed on in this document. According to the document, it seems we have agreed on the following points about the wtf/ => base/ dependency.

======

- wtf/ will depend on base/. This enables Blink to use base/ things. In order to avoid Blink from using random base/ things that have equivalents in wtf/ (e.g., to avoid mixing std::string and WTF::String in Blink), we will whitelist base/ things Blink can use.

- If Blink decides to use base/X and wtf/ has its equivalent, we will remove the equivalent from wtf/.

- Given that Blink and Chromium have different assumptions & priorities on performance, memory and code health, we won’t aggressively try to share implementations of standard libraries between base/ and wtf/ (e.g., String, Vector). This means that we will keep standard libraries in wtf/ until we’re pretty sure that they should be replaced with base/ alternatives or they should replace base/ alternatives (e.g., we plan to replace Chromium’s tcmalloc with PartitionAlloc).

======

Any thoughts?

--

Kentaro Hara, Tokyo, Japan

[Adam Barth]

There's actually a subtle dependency from tracing to std::string that bit us when we removed the platform wrapper for tracing in Sky.  See, for example, this code:

https://github.com/flutter/engine/blob/master/sky/engine/core/frame/Tracing.cpp#L22

I didn't look at what it would take on the base side to remove this dependency.

I'm happy to chat at some point if you'd to hear about our experience doing something similar.

Adam

[Dan Sinclair]

We do allow passing std::string as a convenience and then pull the c style string out of that. Most other things either require a long-lived c string or using the copy macro on a c string like in your example.

Tracing also uses scoped_refptrs to wrap the convertible objects.

Dan

[Dan Sinclair]

Doing a quick grep, it looks like most of the trace points are using c-style strings anyway. Those points wouldn't change at all, we'd just need to change the includes to point to base/trace_event/trace_event.h.

There is one override in TraceEvent.h which converts WTF::CString&  to a string tracing understands by calling .data() on it. Those would have to call .data() at the call site instead of using the wrapper.

There are a few places in the code (devtools, v8binding, resource fetcher, layout) that use ConverteableToTraceFormat. Those would need to return a scoped_refptr instead of a PassRefPtr in order to be compatible with the base tracing system.

There also appear to be a few places using platform/TracedValue. Those would be converted to a TracedValue object inside base/trace_event/trace_event_argument.h (internally this uses a base/pickle.h). TracedValue does take std::string's as parameters. primiano@ would it be possible to use char*'s here? Or have char* variants on these methods? It maybe possible to keep the current TracedValue inside platform, just change it to use the ConvertableToTraceFormat from base instead of the one in platform.

dan

[Primiano Tucci]

> primiano@ would it be possible to use char*'s here? Or have char* variants on these methods?

Technically we could add an overload which takes a char* also. My only concern there would be that, right now, in base::TraceValue char* is used to make the "will not copy" contract explicit. Turning everything into char* will make that a bit less obvious probably.

 

> It maybe possible to keep the current TracedValue inside platform, just change it to use the ConvertableToTraceFormat from base instead of the one in platform.

Hmm that would leave still lot of problems open. base::TracedValue today is based on a base::Pickle, which in turn is memory-dense, fast and makes easy to estimate (w.r.t. memory overhead).

The TracedValue inside blink still handles its own set of dictionary and lists and is really a copy of the old TracedValue that we had in chromium (before the pickle).

If we go through the efforts of a refactoring I'd really like to get rid of blink's copy of TracedValue.

I think all boils down here to: how do we deal when it comes to calling base methods which take a std::string. It this this is going to be a recurring pattern (see my other reply on haraken's thread about this).

Note that I am not suggesting that we should start using std::string in blink. The point is only calling chromium methods from blink that take a std::string.

[Jeremy Roman]

On Mon, Sep 28, 2015 at 10:23 AM, Dan Sinclair <dsin...@chromium.org> wrote:

Doing a quick grep, it looks like most of the trace points are using c-style strings anyway. Those points wouldn't change at all, we'd just need to change the includes to point to base/trace_event/trace_event.h.

There is one override in TraceEvent.h which converts WTF::CString&  to a string tracing understands by calling .data() on it. Those would have to call .data() at the call site instead of using the wrapper.

Presumably we could even salvage that by having setTraceValue be defined for WTF::CString in WTF (e.g. using ADL). (Without needing to explicitly call data().)

[Kentaro Hara]

On Mon, Sep 28, 2015 at 11:47 PM, Primiano Tucci <prim...@chromium.org> wrote:

> primiano@ would it be possible to use char*'s here? Or have char* variants on these methods?

Technically we could add an overload which takes a char* also. My only concern there would be that, right now, in base::TraceValue char* is used to make the "will not copy" contract explicit. Turning everything into char* will make that a bit less obvious probably.

 

> It maybe possible to keep the current TracedValue inside platform, just change it to use the ConvertableToTraceFormat from base instead of the one in platform.

Hmm that would leave still lot of problems open. base::TracedValue today is based on a base::Pickle, which in turn is memory-dense, fast and makes easy to estimate (w.r.t. memory overhead).

The TracedValue inside blink still handles its own set of dictionary and lists and is really a copy of the old TracedValue that we had in chromium (before the pickle).

If we go through the efforts of a refactoring I'd really like to get rid of blink's copy of TracedValue.

Removing the blink's copy of TracedValue sounds reasonable to me anyway. When we introduced the blink's TracedValue, what we really wanted to do was to reuse the infrastructure of the base::Pickle (but we couldn't because the repository merge was not yet there).

[Oystein Eftevaag]

I hacked together a preliminary CL to get the full overview of what's needed: https://codereview.chromium.org/1381023002

A few notes:

* I left TracedValue in place and inheriting directly from base::trace_event::ConvertableToTraceFormat, as a first step.

* base/location.h is used by base/trace_event and FROM_HERE conflicts with WebLocation; right now it compiles via an #ifdef hack but getting rid of WebLocation may be a prerequisite.

* In the interest of CL brevity I added a char*() implicit convertor to CString; I agree the call sites should change to pass const char* instead.

* Doing this revealed a few inconsistencies between the Blink and the base tracing macros (again), which should be fixed regardless.

Other than that, it seems reasonably straightforward?

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

[Dan Sinclair]

Does the above patch and required changes seem acceptable to people? Can we start moving ahead with using base/trace_event from blink directly?

Thanks,

dan

[Daniel Cheng]

Will the rule for using scoped_refptr vs RefPtr be "use scoped_refptr for base:: objects, and RefPtr for everything else"?

Also, will we try to converge the behavior/conventions of the two types at some point?
- base::RefCounted objects start with a refcount of zero and should be immediately be stored in a scoped_refptr, but WTF::RefCounted objects start with one reference that should be immediately adopted.
- scoped_refptr is often passed by copy or const ref, while RefPtr usually tries to move references to avoid refcount churn. Do we care? Will it have a measurable impact where we use trace events? This is largely a hypothetical question for other patches that use scoped_refptr: the draft cl is actually an improvement in this regard. With oysteine@'s patch, the compiler will now use RVO to completely elide the copy, whereas it was forced to use move semantics before.

Daniel

[Oystein Eftevaag]

On Mon, Oct 5, 2015 at 12:19 PM Daniel Cheng <dch...@chromium.org> wrote:

Will the rule for using scoped_refptr vs RefPtr be "use scoped_refptr for base:: objects, and RefPtr for everything else"?

Slightly expanded, we'd also need a rule for what to do about refcounted Blink objects that inherit from something in base::, as with TracedValue in my CL.

[Dana Jansens]

On Mon, Oct 5, 2015 at 3:19 PM, Daniel Cheng <dch...@chromium.org> wrote:

Will the rule for using scoped_refptr vs RefPtr be "use scoped_refptr for base:: objects, and RefPtr for everything else"?

Also, will we try to converge the behavior/conventions of the two types at some point?

I'd love to see it.

- base::RefCounted objects start with a refcount of zero and should be immediately be stored in a scoped_refptr, but WTF::RefCounted objects start with one reference that should be immediately adopted.

 This has some interesting interaction with skia. Skia refcounted things start at 1 similar to WTF, so in blink we store skia objects in RefPtr. But in chromium we use a skia::RefPtr. If all else is equal, it may be nice to make base's RefCounted act like WTF so that we can remove skia::RefPtr. TBH I'm not sure of the pros/cons of the two approaches outside of this right now. Maybe someone could write a detailed proposal.

- scoped_refptr is often passed by copy or const ref, while RefPtr usually tries to move references to avoid refcount churn. Do we care? Will it have a measurable impact where

scoped_refptr has a Pass() function so you can pass by value and move the reference. So I think they are equivalent types in spirit now.

[Dana Jansens]

On Mon, Oct 5, 2015 at 7:22 PM, Oystein Eftevaag <oyst...@google.com> wrote:

On Mon, Oct 5, 2015 at 12:19 PM Daniel Cheng <dch...@chromium.org> wrote:

Will the rule for using scoped_refptr vs RefPtr be "use scoped_refptr for base:: objects, and RefPtr for everything else"?

Slightly expanded, we'd also need a rule for what to do about refcounted Blink objects that inherit from something in base::, as with TracedValue in my CL.

Could we make the compiler enforce the right usage? WTF::RefPtr should be used on things that are WTF::RefCounted. scoped_refptr should be used on things that are base::RefCounted{ThreadSafe}. How many exceptions are there to this rule?

[Jeremy Roman]

I'm pretty sure the compiler already enforces this. Unless you inherit from both, but then you are a terrible person.

[Oystein Eftevaag]

I guess the question is basically, are we okay with Blink types using scoped_refptr if they inherit from a base/ object, or should they be using a wrapper in this case.

[Oystein Eftevaag]

(The compiler does enforce this right now, since the Add/DecRef methods differ between the two implementations)

[Dan Sinclair]

Elliott, did you have any comments on Oysteins patch (https://codereview.chromium.org/1381023002)? Are you OK with us moving forward with this as is?

Thanks,

dan

[Elliott Sprehn]

At this time I don't think we want base/ to be used outside platform (and WTF). I would suggest moving tracing integration into WTF with wrappers, it'll remove considerable amounts of plumbing and allow tighter integration of WTF types.

At a future time once we've got experience with base in platform and WTF we can consider using it more widely.

[Primiano Tucci]

What happen to the plan of "[blink-dev] A guideline for introducing Blink => Chromium dependencies" of whitelisting headers on a case-per-case basis? Should we consider that not valid anymore? 

The last words there, on Oct 5, were

> please be patient, I'm going to share a plan and guidance in the next few days. :)

What happened to that?

[Elliott Sprehn]

On Wed, Oct 14, 2015 at 10:58 AM, Primiano Tucci <prim...@chromium.org> wrote:

What happen to the plan of "[blink-dev] A guideline for introducing Blink => Chromium dependencies" of whitelisting headers on a case-per-case basis? Should we consider that not valid anymore? 

I'd like for us to step back and focus on a smaller part of the codebase for now, Oystien's patch involves putting scoped_refptr all over core/, which I don't think we're ready for.

 

The last words there, on Oct 5, were

> please be patient, I'm going to share a plan and guidance in the next few days. :)

What happened to that?

I'm working on it, please be patient. We've done fine for 7 years with the current level of separation, waiting a bit to come up with a plan isn't going to cause any issues.

- E