The IWYU fallout of passing scoped_refptr<T> by value.

[Gabriel Charette]

tl;dr; by IWYU (include-what-you-use), in order to take or return scoped_refptr<T> by value, one also needs to #include the full definition of T.

In a prior discussion this February, we decided that it was preferable to pass scoped_refptr<T> by value as it allows a reference move without reference bumping.

I just discovered that this had a subtle side-effect...

Passing scoped_refptr<T> by value now requires the full definition of T as the ref-count is not held inside the scoped_refptr but on T itself (in its RefCounted or RefCountedThreadSafe portion), so T needs to be defined merely to know that it implements RefCounted(ThreadSafe).

Interestingly, ref_counted_unittest.cc does verify that using an opaque type works but it requires that the template be fwd-declared as extern template class scoped_refptr<base::OpaqueRefCounted>. Such a fwd-declaration is impractical because it requires an instantiation of that extern decl in the same compilation unit (which works in isolation cases like opaque_ref_counted.(cc|h) but doesn't in headers used across compilation units..).

It turns out that there are zero uses of the extern fwd-decl paradigm in our codebase outside of base/test/opaque_ref_counted.h. Furthermore, the style guide frowns upon complex fwd-decls so I guess this test is proving that a paradigm that no one uses nor should use is possible...

I tried but failed to tweak ref_counted.h to allow AddRef/Release to be called on T without knowing its full type (and also failed to make it work for use cases that don't even require AddRef/Release).

Conclusion: by IWYU (include-what-you-use), in order to take or return scoped_refptr<T> by value, one also needs to #include the full definition of T (as was painfully discovered when trying to remove a single include from sequenced_worker_pool.h resulting in 87 files worth of IWYU fixes :-O!)

PS: after discussion with other people, the only potential solution we can think of would be to move the static AddRef/Release calls in ref_counted.h out of line. <hand-waving> this would prevent it from being inlined and would ensure scoped_refptr<T> only needs to be fully defined once per compilation unit -- not clear who would be in charge of defining it </hand-waving>. The performance cost of not having this inlined is likely not worth it either to save a few includes (again, the style guide frowns upon fwd-decls already).

Cheers!

Gab

[Jeremy Roman]

This is how things work with std::unique_ptr: if ~unique_ptr is called (it is if you have a local unique_ptr variable, including a parameter), you need the definition of T in order to call ~T (even if the compiler can statically prove that the T* will actually be null in practice). I'd expect scoped_refptr to behave similarly.

Is this a big problem in practice? I'd have expected most code which accepts an owning reference to T to already need its definition (e.g., to call its methods). Yes, you could extract things out into an out-of-line function per-type, but there are traps here, too. Either you'll have to make it a template sensitive enough to not match derived types (e.g. by making it a static method on scoped_refptr<T>) but then will have to define this for every derived class that might have a scoped_refptr to it, or you'll have to make it an overloaded function (in which case you might even end up with no matching overload or even possibly the wrong overload selected). It seems pretty messy to me.

My preference would be "just include the header".

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Peter Kasting]

On Tue, Nov 1, 2016 at 7:30 AM, Gabriel Charette <g...@chromium.org> wrote:

Passing scoped_refptr<T> by value now requires the full definition of T as the ref-count is not held inside the scoped_refptr but on T itself (in its RefCounted or RefCountedThreadSafe portion), so T needs to be defined merely to know that it implements RefCounted(ThreadSafe).

If we really found this onerous, I suppose it might be an argument for std::shared_ptr<>, although that debate is thorny.

Interestingly, ref_counted_unittest.cc does verify that using an opaque type works but it requires that the template be fwd-declared as extern template class scoped_refptr<base::OpaqueRefCounted>. Such a fwd-declaration is impractical because it requires an instantiation of that extern decl in the same compilation unit (which works in isolation cases like opaque_ref_counted.(cc|h) but doesn't in headers used across compilation units..).

It turns out that there are zero uses of the extern fwd-decl paradigm in our codebase outside of base/test/opaque_ref_counted.h. Furthermore, the style guide frowns upon complex fwd-decls so I guess this test is proving that a paradigm that no one uses nor should use is possible...

Sounds like we should remove that class/test.

PS: after discussion with other people, the only potential solution we can think of would be to move the static AddRef/Release calls in ref_counted.h out of line. <hand-waving> this would prevent it from being inlined and would ensure scoped_refptr<T> only needs to be fully defined once per compilation unit -- not clear who would be in charge of defining it </hand-waving>. The performance cost of not having this inlined is likely not worth it either to save a few includes (again, the style guide frowns upon fwd-decls already).

Note that it's mostly the Google style guide which frowns on forward decls, the Chromium style guide explicitly countermands that advice.

I don't think it's necessary to try to move these out-of-line; as Jeremy notes, this is sort of expected behavior when working with smart pointer types, and seems like an OK restriction to have in the codebase.

PK 

[Gabriel Charette]

On Tue, Nov 1, 2016 at 1:43 PM Jeremy Roman <jbr...@chromium.org> wrote:

This is how things work with std::unique_ptr: if ~unique_ptr is called (it is if you have a local unique_ptr variable, including a parameter), you need the definition of T in order to call ~T (even if the compiler can statically prove that the T* will actually be null in practice). I'd expect scoped_refptr to behave similarly.

Is this a big problem in practice? I'd have expected most code which accepts an owning reference to T to already need its definition (e.g., to call its methods).

Look at https://codereview.chromium.org/2443103003/, there are many places that merely forward the ref to another object.

[Dmitry Skiba]

If we're OK with adding one more pointer to scoped_refptr (+level of indirection), then we can hide AddRef/Release behind a function that is only touched in ctor / ptr assignment: https://codereview.chromium.org/2471043002/

Could only test on OpaqueRefCounted from unittests, but everything compiles with fwd-decls removed.

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Alexey Baskakov]

We could try something like this (similar to boost intrusive_ptr approach):

template<class T> class intrusive_ptr

{

private:

    typedef intrusive_ptr this_type;

public:

    typedef T element_type;

    intrusive_ptr() : px( 0 )

    {

    }

    intrusive_ptr( T * p, bool add_ref = true ): px( p )

    {

        if( px != 0 && add_ref ) intrusive_ptr_add_ref( px );

    }

    template<class U>

    intrusive_ptr( intrusive_ptr<U> const & rhs )

    : px( rhs.get() )

    {

        if( px != 0 ) intrusive_ptr_add_ref( px );

    }

    intrusive_ptr(intrusive_ptr const & rhs): px( rhs.px )

    {

        if( px != 0 ) intrusive_ptr_add_ref( px );

    }

    ~intrusive_ptr()

    {

        if( px != 0 ) intrusive_ptr_release( px );

    }

    

    T* get() { return px; }

private:

    T * px;  

};

// forward declarations

class MyClass;

template<typename T>

class RefCounted {

public:

  void AddRef() { ++c; }

  void Release() {

    --c;

    if (c == 0)

      delete static_cast<const T*>(this);    

  }

  

  int c = 0;

};

int foo(intrusive_ptr<MyClass> ptr) {

  ptr.get();

  return 0;

}

void intrusive_ptr_add_ref(MyClass * p);

void intrusive_ptr_release(MyClass * p);

class MyClass : public RefCounted<MyClass> {

  int a;

};

 inline void intrusive_ptr_add_ref(MyClass * p)

  {

     p->AddRef();

  }

 inline void intrusive_ptr_release(MyClass * p)

  {

     p->Release();

  } 

int main() {

  intrusive_ptr<MyClass> ptr(new MyClass);

  foo(ptr);

  return 0;

}

[Jeremy Roman]

This approach has the other consequence I mentioned: if you have:

class MyDerivedClass : public MyClass {};

Then this will fail to compile unless you also define the addref/release functions for the derived class (and forward-declare them where you want to use them), or you include the definition of MyDerivedClass (so that overload resolution can tell that it derives MyClass).

I think there's also the potential for missing optimization opportunities because the AddRef call could probably be inlined, but if its definition is not available, then inlining won't be possible until LTO (which I don't think we do everywhere yet?).

 

On Wednesday, November 2, 2016 at 5:46:19 PM UTC+11, Dmitry Skiba wrote:

If we're OK with adding one more pointer to scoped_refptr (+level of indirection), then we can hide AddRef/Release behind a function that is only touched in ctor / ptr assignment: https://codereview.chromium.org/2471043002/

Do we really want to make each scoped_refptr take twice as much memory in order to avoid these #includes?

[Antoine Labour]

Uh, please, no, seriously. Don't punish our users to make developers' lives easier.

Antoine

[Dmitry Skiba]

On Wed, Nov 2, 2016 at 7:44 AM, Jeremy Roman <jbr...@chromium.org> wrote:

Probably not. After all scoped_refptr has never supported incomplete types, and it was never a problem.

This thread started with a note of a failed attempt to make scoped_refptr play nice with incomplete types, so I demonstrated that it can be done. It can probably be done differently (making AddRef/Release virtual for example), but there will always be a downside of some sort, because obviously the current implementation is the optimal one.

 

[Trent Apted]

On 2 November 2016 at 04:54, Peter Kasting <pkas...@chromium.org> wrote:

Interestingly, ref_counted_unittest.cc does verify that using an opaque type works but it requires that the template be fwd-declared as extern template class scoped_refptr<base::OpaqueRefCounted>. Such a fwd-declaration is impractical because it requires an instantiation of that extern decl in the same compilation unit (which works in isolation cases like opaque_ref_counted.(cc|h) but doesn't in headers used across compilation units..).

It turns out that there are zero uses of the extern fwd-decl paradigm in our codebase outside of base/test/opaque_ref_counted.h. Furthermore, the style guide frowns upon complex fwd-decls so I guess this test is proving that a paradigm that no one uses nor should use is possible...

Sounds like we should remove that class/test.

I think there is one in ipc_message_macros.h . And by "one" I mean every IPC thingy that uses a IPC_MESSAGE_DECL and gets:

  extern template class EXPORT_TEMPLATE_DECLARE(IPC_MESSAGE_EXPORT) \

      IPC::MessageT<msg_name##_Meta>;                               \

But there's a minefield of compiler bugs around this. See http://crbug.com/623844 - e.g. gcc fixed one bug only two months ago.

So ipc/export_template.h might help make some crazy things possible. But it's full of dragons. And may need yet more hacks.

Sorry I don't have more suggestions apart from "you probably don't want extern templates". I think it kinda works in the test files since they're always statically linked, but when shared linking is involved it becomes a horror.

[Daniel Cheng]

We use extern template decls in several other places throughout the codebase (logging, StringPiece, dbus) so we don't end up with a bunch of template instantiations that are (hopefully) collapsed together at the end. But yes, it's very tricky when it has to be mixed with exports in a component build. I would highly recommend not using them unless there's a good reason to.

Daniel