Closed Bug 1198451 Opened 4 years ago Closed 4 years ago

Create a type erased function/callable type

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jrmuizel, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(3 files)

This would be valuable for accepting lambdas
Whiteboard: gfx-noted
Such a type should be in MFBT.
Component: Graphics → MFBT
Bug 1198451 - Example usage of mozilla::Function in APZ code
Here's a crack at implementing such a wrapper, and an example use in APZ code. I want to add some tests and clean it up a bit before submitting it for review.
Assignee: nobody → botond
I was curious about other implementations. libcxx seems to use 3*sizeof(void*) of built-in storage https://github.com/llvm-mirror/libcxx/blob/6d9da5891f6628ffa9d2f382113a8231121aeee1/include/functional#L1564
Hi Botond,

I have some thought about this patch,

1. Can we remove the |explicit| keyword on Function constructor? We can construct the Function like
   Funcion<void()> foo = func;

2. http://ideone.com/mThy8i
   I just simplified the code and please see the result.
   you can find that, in our design, we have an extra |move| since we used forward "twice" but     std::function not.
   I cannot figure out how to prevent from the extra move......do you have any idea?

3. Does it support |pointer to member function| or it will revise in the future.

Thank you.
(In reply to James Cheng[:JamesCheng] from comment #6)
> 1. Can we remove the |explicit| keyword on Function constructor? We can
> construct the Function like
>    Funcion<void()> foo = func;

You're right, we should probably follow std::function's interface here and make the constructor implicit.

> 2. http://ideone.com/mThy8i
>    I just simplified the code and please see the result.
>    you can find that, in our design, we have an extra |move| since we used
> forward "twice" but     std::function not.
>    I cannot figure out how to prevent from the extra move......do you have
> any idea?

Do you know what compiler and compiler version this uses? Testing locally with gcc 4.9.2, I see two moves for std::function as well.

> 3. Does it support |pointer to member function| or it will revise in the
> future.

It doesn't support pointers to member functions directly, but it supports arbitrary function objects, so it can be nicely combined with an adapter that wraps pointers to member functions into function objects, like std::mem_fn or an equivalent:

  Function<Signature> func = std::mem_fn(myMemberFunctionPointer);

If we'd like Function to do this adaptation automatically, we can explore doing that as a follow-up.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> I was curious about other implementations. libcxx seems to use
> 3*sizeof(void*) of built-in storage
> https://github.com/llvm-mirror/libcxx/blob/
> 6d9da5891f6628ffa9d2f382113a8231121aeee1/include/functional#L1564

I guess that's an optimization to avoid a dynamic allocation and just store the wrapped function object in-line if it's small enough. We can explore doing that if you think it's worth it; I'm inclined to leave it for a follow-up, though.
(In reply to Botond Ballo [:botond] from comment #8)
> I guess that's an optimization to avoid a dynamic allocation and just store
> the wrapped function object in-line if it's small enough. We can explore
> doing that if you think it's worth it; I'm inclined to leave it for a
> follow-up, though.

Please.
(In reply to Botond Ballo [:botond] from comment #7)
> (In reply to James Cheng[:JamesCheng] from comment #6)
> > 1. Can we remove the |explicit| keyword on Function constructor? We can
> > construct the Function like
> >    Funcion<void()> foo = func;
> 
> You're right, we should probably follow std::function's interface here and
> make the constructor implicit.
Thanks.
> 
> > 2. http://ideone.com/mThy8i
> >    I just simplified the code and please see the result.
> >    you can find that, in our design, we have an extra |move| since we used
> > forward "twice" but     std::function not.
> >    I cannot figure out how to prevent from the extra move......do you have
> > any idea?
> 
> Do you know what compiler and compiler version this uses? Testing locally
> with gcc 4.9.2, I see two moves for std::function as well.
Sorry I don't know this online compiling environment. 
My gcc 4.9.2 got the same result as yours. I just curious about how to save one extra move.

> 
> > 3. Does it support |pointer to member function| or it will revise in the
> > future.
> 
> It doesn't support pointers to member functions directly, but it supports
> arbitrary function objects, so it can be nicely combined with an adapter
> that wraps pointers to member functions into function objects, like
> std::mem_fn or an equivalent:
> 
>   Function<Signature> func = std::mem_fn(myMemberFunctionPointer);
> 
> If we'd like Function to do this adaptation automatically, we can explore
> doing that as a follow-up.
OK, thanks. 
Your Function implementation can accept all callable type. But do we encourage to use STL in gecko since I cannot find MFBT that implements std::bind?
Comment on attachment 8654653 [details]
MozReview Request: Bug 1198451 - Add a type-erased callable wrapper, mozilla::Function, to MFBT. r=froydnj

Bug 1198451 - Add a type-erased callable wrapper, mozilla::Function, to MFBT. r=froydnj
Attachment #8654653 - Attachment description: MozReview Request: Bug 1198451 - Add a type-erased callable wrapper to MFBT → MozReview Request: Bug 1198451 - Add a type-erased callable wrapper, mozilla::Function, to MFBT. r=froydnj
Attachment #8654653 - Flags: review?(nfroyd)
Comment on attachment 8654654 [details]
MozReview Request: Bug 1198451 - Tests for mozilla::Function. r=froydnj

Bug 1198451 - Tests for mozilla::Function. r=froydnj
Attachment #8654654 - Attachment description: MozReview Request: Bug 1198451 - Example usage of mozilla::Function in APZ code → MozReview Request: Bug 1198451 - Tests for mozilla::Function. r=froydnj
Attachment #8654654 - Flags: review?(nfroyd)
Bug 1198451 - Disambiguate mozilla::dom::Function from mozilla::Function. r=froydnj
Attachment #8657664 - Flags: review?(nfroyd)
The updated patches add some explanatory comments, make the constructor implicit as discussed in comment 7, and add tests.

The "example use" patch I'll move to bug 1202312.
Blocks: 1202312
Comment on attachment 8654654 [details]
MozReview Request: Bug 1198451 - Tests for mozilla::Function. r=froydnj

https://reviewboard.mozilla.org/r/17725/#review16599
Attachment #8654654 - Flags: review?(nfroyd) → review+
Comment on attachment 8657664 [details]
MozReview Request: Bug 1198451 - Disambiguate mozilla::dom::Function from mozilla::Function. r=froydnj

https://reviewboard.mozilla.org/r/18383/#review16601
Attachment #8657664 - Flags: review?(nfroyd) → review+
Attachment #8654653 - Flags: review?(nfroyd) → review+
Comment on attachment 8654653 [details]
MozReview Request: Bug 1198451 - Add a type-erased callable wrapper, mozilla::Function, to MFBT. r=froydnj

https://reviewboard.mozilla.org/r/17723/#review16597

::: mfbt/Function.h:28
(Diff revision 2)
> +// This class is intended to provide functionality similar to the C++11

Nit: I feel like there are several distinct topics in this block of text.  Can you introduce some paragraph breaks to group things?  Suggested breaks:

|Function<Signature>| is a wrapper...

Supported callable types include...

|Signature| is a type of the form...

This class is intended...

::: mfbt/Function.h:92
(Diff revision 2)
> +  // TODO: Consider implementing a small object optimization.

Could you please file a followup bug on this?
Eagerly anticipating this.
Addressed review comments locally. Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bbd21c01c77
(In reply to Botond Ballo [:botond] from comment #19)
> Addressed review comments locally. Try push:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bbd21c01c77

Had some build errors, fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fafb2e10d961
(In reply to Botond Ballo [:botond] from comment #20)
> Had some build errors, fixed:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=fafb2e10d961

Gah, this is still failing to build on Windows, because MSVC doesn't auto-generate move constructors...
I was surprised that our UniquePtr declares the deleted copy construct and operator assignment in private field. It should be in public field right?
(In reply to James Cheng[:JamesCheng] from comment #22)
> I was surprised that our UniquePtr declares the deleted copy construct and
> operator assignment in private field. It should be in public field right?

This may have been from a time when |= delete| wasn't supported on all our compilers, so making them private members would lessen the chances of them being used inadvertently.
Hi Nathan,
So it should be moved into public field now, right?

Hi Botond,

That's just my two cent...

I think the move constructor should not be implicitly generated.

I referenced http://en.cppreference.com/w/cpp/language/move_constructor  ([Implicitly-declared move constructor]).

g++4.9+ and VS2015 disobey the rule I don't know why...(maybe the spec is updated?)

I am not quite sure why the compile error is. I guess it is |Function| = |Function| cannot find its move constructor, so they want to do the |Copy|. But your implementation is using UniqePtr which is not allowed copy.... so the error occurred. Am I right?

Should we consider not to use UniquePtr and use copyable smart pointer instead?

If using UniquePtr, that means the Function object cannot be copied. 

I'm not sure if my understanding is correct, please let me know if my understanding is incorrect.

Thank you.
After a couple of more tries, I've got it to build on MSVC:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f344f0205f59

Now I just need to wait for the trees to open.
(In reply to James Cheng[:JamesCheng] from comment #24)
> I think the move constructor should not be implicitly generated.
> 
> I referenced http://en.cppreference.com/w/cpp/language/move_constructor 
> ([Implicitly-declared move constructor]).
> 
> g++4.9+ and VS2015 disobey the rule I don't know why...(maybe the spec is
> updated?)

My reading of that section is that a move constructor *should* be implicitly generated, and it is with gcc/clang, but not with MSVC (at least, not with the version used in automation, which I guess is 2013; maybe 2015 finally started doing it properly).

> I am not quite sure why the compile error is. I guess it is |Function| =
> |Function| cannot find its move constructor, so they want to do the |Copy|.
> But your implementation is using UniqePtr which is not allowed copy.... so
> the error occurred. Am I right?

It looks like MSVC doesn't generate the move constructor, but it does generate the copy constructor (which is ill-formed), and tries to use it.

> Should we consider not to use UniquePtr and use copyable smart pointer
> instead?
> 
> If using UniquePtr, that means the Function object cannot be copied. 

Yes, we should consider that, as a follow-up enhancement. We have to figure out what semantics we want. (For example, if we use an nsRefPtr then different copies will point to the same callable object - is that desirable?)
Blocks: 1204160
(In reply to Nathan Froyd [:froydnj] from comment #17)
> ::: mfbt/Function.h:92
> (Diff revision 2)
> > +  // TODO: Consider implementing a small object optimization.
> 
> Could you please file a followup bug on this?

Filed bug 1204160.
(In reply to Botond Ballo [:botond] from comment #26)
> > Should we consider not to use UniquePtr and use copyable smart pointer
> > instead?
> > 
> > If using UniquePtr, that means the Function object cannot be copied. 
> 
> Yes, we should consider that, as a follow-up enhancement. We have to figure
> out what semantics we want. (For example, if we use an nsRefPtr then
> different copies will point to the same callable object - is that desirable?)

I filed 1204166 for this.
Thanks for feedback.

(In reply to Botond Ballo [:botond] from comment #26)
> (In reply to James Cheng[:JamesCheng] from comment #24)
> > I think the move constructor should not be implicitly generated.
> > 
> > I referenced http://en.cppreference.com/w/cpp/language/move_constructor 
> > ([Implicitly-declared move constructor]).
> > 
> > g++4.9+ and VS2015 disobey the rule I don't know why...(maybe the spec is
> > updated?)
> 
> My reading of that section is that a move constructor *should* be implicitly
> generated, and it is with gcc/clang, but not with MSVC (at least, not with
> the version used in automation, which I guess is 2013; maybe 2015 finally
> started doing it properly).

But the website said |all of the following is true:| ... I'm not sure why MSVC 2015 and g++ will implicit declare move constructor when you explicitly implement other constructors?

> 
> > I am not quite sure why the compile error is. I guess it is |Function| =
> > |Function| cannot find its move constructor, so they want to do the |Copy|.
> > But your implementation is using UniqePtr which is not allowed copy.... so
> > the error occurred. Am I right?
> 
> It looks like MSVC doesn't generate the move constructor, but it does
> generate the copy constructor (which is ill-formed), and tries to use it.
>
So could we write a workaround move constructor like

Function(Function&&) = default; to avoid the VC2013 compile error.

The current Function cannot be explicit move on VS2013 environment right?

Does it make sense?
 
> > Should we consider not to use UniquePtr and use copyable smart pointer
> > instead?
> > 
> > If using UniquePtr, that means the Function object cannot be copied. 
> 
> Yes, we should consider that, as a follow-up enhancement. We have to figure
> out what semantics we want. (For example, if we use an nsRefPtr then
> different copies will point to the same callable object - is that desirable?)
(In reply to James Cheng[:JamesCheng] from comment #30)
> (In reply to Botond Ballo [:botond] from comment #26)
> > (In reply to James Cheng[:JamesCheng] from comment #24)
> > > I think the move constructor should not be implicitly generated.
> > > 
> > > I referenced http://en.cppreference.com/w/cpp/language/move_constructor 
> > > ([Implicitly-declared move constructor]).
> > > 
> > > g++4.9+ and VS2015 disobey the rule I don't know why...(maybe the spec is
> > > updated?)
> > 
> > My reading of that section is that a move constructor *should* be implicitly
> > generated, and it is with gcc/clang, but not with MSVC (at least, not with
> > the version used in automation, which I guess is 2013; maybe 2015 finally
> > started doing it properly).
> 
> But the website said |all of the following is true:| ... I'm not sure why
> MSVC 2015 and g++ will implicit declare move constructor when you explicitly
> implement other constructors?

The list is:

 - there are no user-declared copy constructors
 - there are no user-declared copy assignment operators
 - there are no user-declared move assignment operators
 - there are no user-declared destructors 

There is no prohibition against user-declared constructors that are not copy constructors.

> > > I am not quite sure why the compile error is. I guess it is |Function| =
> > > |Function| cannot find its move constructor, so they want to do the |Copy|.
> > > But your implementation is using UniqePtr which is not allowed copy.... so
> > > the error occurred. Am I right?
> > 
> > It looks like MSVC doesn't generate the move constructor, but it does
> > generate the copy constructor (which is ill-formed), and tries to use it.
> >
> So could we write a workaround move constructor like
> 
> Function(Function&&) = default; to avoid the VC2013 compile error.

Yep, that's the fix I applied in the patch that I landed (except I implemented the move constructor myself instead of using |= default|; I seem to recall MSVC not liking |= default|, either, though perhaps that's since been fixed.)

> The current Function cannot be explicit move on VS2013 environment right?

I'm not sure what you mean by "explicit move".
Blocks: 1212745
Blocks: 1204166
Duplicate of this bug: 1164522
You need to log in before you can comment on or make changes to this bug.