Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce Event.h and EventQueue.h using C++11 #10895

Merged
merged 3 commits into from Jul 15, 2019

Conversation

@kjbracey-arm
Copy link
Contributor

commented Jun 25, 2019

Description

Variadic templates can reduce Event.h from 4,100 lines to 300, and EventQueue.h from 3,400 to 1,000, so 6,000 lines saved total.

End result isn't totally variadic, as we still need specialisations for storing 0-5 values in contexts, but that specialisation is now in exactly one place.

Only change other from switching to variadic templates is using delegating constructors instead of the new (this) trick. That trick is still used in the assignment operator.

Minor documentation correction. It's possible that the separate simplified variadic Doxygen version not be needed now, but I've left it.

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@pan-, @bulislaw, @geky, @evedon

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:event_variadic branch from 0ea2888 to d6151c7 Jun 25, 2019
Variadic templates can reduce Event.h from 4,100 lines to 300, and
EventQueue.h from 3,400 to 1,000, so 6,000 lines saved total.

End result isn't totally variadic, as we still need specialisations
for storing 0-5 values in contexts, but that specialisation is now
in exactly one place.

Only change other from switching to variadic templates is using
delegating constructors instead of the `new (this)` trick. That trick is
still used in the assignment operator.

Minor documentation correction. It's possible that the separate
simplified variadic Doxygen version not be needed now, but I've left it.
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:event_variadic branch from d6151c7 to 4d9148a Jun 25, 2019
@ciarmcom ciarmcom requested review from bulislaw, evedon, geky, pan- and ARMmbed/mbed-os-maintainers Jun 25, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Second commit makes a change to template deduction which I think is required to make all variadic cases work, but it will be a subtle behaviour change. Full details in commit message - review especially from @geky appreciated.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:event_variadic branch from 2ff14df to 5babe7e Jun 25, 2019
Previous commit just replaced instances of "class B0, class B1, class
C0, class C1" with "class... BoundArgs, class... ContextArgs".

This loses the requirement that the numbers must match.

Now, the original code was also inconsistent as to whether it used
separate types for the target function and the call input parameters.
Some forms just used B0, B1 as parameters rather than separate C0, C1.

I believe the separate parameters would have been primarily to avoid
template deduction confusion - eg if int was supplied to a B0 parameter
but the function took char as B0, there would be an ambiguity. But the
fix didn't seem to be fully applied.

Rewritten all templates parameterising on function pointer type and
input arguments so that they use `type_identity_t<BoundArgTs>...` as
input parameters to match the target function.

This has the subtle effect that any conversion happens at invocation,
before storing to the context, rather than when the context calls the
target.
@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:event_variadic branch from 5babe7e to 62062ef Jun 25, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Hmm, still have a problem with the template deduction - I see now that if you don't let the passed arguments be freely-typed, any need for conversion makes the (F, any...) (0 conversions) match better than (obj, method(types...), types...) (1 or more args converted).

But then if they're freely typed, I don't see how I can use the variadic form for

template <typename T, typename R, typename... BoundArgTs, typename... ArgTs, typename... ContextArgTs>
Event<void(ArgTs...)> EventQueue::event(T *obj, R(T::*method)(BoundArgTs..., ArgTs...), ContextArgTs... context_args)

There's nothing to associate the number of parameters passed to how many bound args we want. It ends up putting nothing in BoundArgTs and everything in ArgTs.

It may be necessary to do these binding calls non-variadically.

template <typename R, typename B0, typename C0, typename A0, typename A1, typename A2>
Event<void(A0, A1, A2)> event(mbed::Callback<R(B0, A0, A1, A2)> cb, C0 c0);

/** Creates a

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jun 26, 2019

Author Contributor

I believe the variadic form of this would be:

template <typename F, typename... ContextTypes>
struct context {
    F f;
    std::tuple<ContextTypes...> c;
    
    context(F f, ContextTypes... c)
        : f(f), c(c...) {}

    template <size_t... I, typename... ArgTs>
    void do_call(std::index_sequence<I...>, ArgTs... args)
    {
        f(get<I>(c)..., args...);
    }
    template <typename... ArgTs>
    void operator()(ArgTs... args)
    {
        do_call(std::index_sequence_for<ContextTypes>(), args...);
    }
};

But that would involve writing std::tuple for ARM C 5.

This comment has been minimized.

Copy link
@pan-

pan- Jun 26, 2019

Member

It just requires a small subset of tuple that shouldn't be hard to implement:

template<typename... Ts>
struct Tuple : TupleImpl<std:: index_sequence_for <Ts...>, Ts...>;

template<typename Indexes, typename... Ts>
struct TupleImpl; 

template<size_t... Indexes, typename... Ts>
struct TupleImpl<std::index_sequence<Indexes...>, Ts...> : : TupleLeaf<Indexes, Ts>... { };

template<size_t Index, typename T>
struct TupleLeaf { 
    T value;
    T& get();
};

template<size_t Index, typename T, typename... Ts>
struct nth_element_impl {
    using type = typename nth_element_impl<Index-1, Ts...>::type;
};

template<typename T, typename... Ts>
struct nth_element_impl<0, T, Ts...> {
    using type = T;
};

template<size_t Index, typename... Ts>
using nth_element = typename nth_element_impl<Index, Ts...>::type;

template<size_t index, typename... Ts>
auto get(const Tuple<Ts...>& t) -> nth_element<index, Ts...> { 
    return static_cast<TupleLeaf<index, nth_element<index, Ts...>>>(t).get();
}  

We can do that latter if it is necessary.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jun 26, 2019

Author Contributor

Don't see any pressing need here - the non-variadic repetition is small and self-contained, and easier to understand.

But I would appreciate any tips on avoiding the bound argument repetition I've just put back, or the (-,C,V,CV) quads on every member function pointer. Those two multiply together nastily.

This comment has been minimized.

Copy link
@pan-

pan- Jul 11, 2019

Member

I think we could use a traits to test the compatibility between the pointer and the function to bind.
Rules are fairly simple:

  • ptr const: reject non const member function
  • ptr volatile: reject non volatile member function

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Jul 11, 2019

Author Contributor

Hmm, I guess maybe you get it nearly for free with is_invocable. (Having paid the cost there).

As I've implemented that now, I'll look at applying it.

This comment has been minimized.

Copy link
@pan-

pan- Jul 11, 2019

Member

It seems to work: https://godbolt.org/z/wGCijo

Unwind previous commit and restore original behaviour for binding,
pending further investigation.

Some functions like `EventQueue::call` require precisely matching
argument types to get the correct overload, as before.

Others like `EventQueue::event` permit compatible types for binding, and
those handle the 0-5 bound arguments non-variadically in order to
correctly locate the free arguments in deduction.
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Okay, I've gone back to non-variadic binding, putting back all the type deduction logic as it was. Did it as a separate commit so the interested can still see what didn't work.

If this is the final version, we can squash it all together.

Means an extra 500 lines, but it's still a 5,500 line saving.

Copy link
Member

left a comment

I generally like the direction, but will leave the detail review for @pan- and @geky

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:event_variadic branch 2 times, most recently from dd55353 to 013377a Jul 10, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@pan-, @geky, this is ready to go.

@pan-
pan- approved these changes Jul 11, 2019
Copy link
Member

left a comment

Looks good to me. We can add optimisations (perfect forwarding can be added to many places) once this is in.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Yes, I'll be sending more updates presently.

My immediate priority is to rework "mbed_cxxsupport.h" before it gets locked in for 5.14 - want to get some better structure there. (See #10274 (comment))

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 12, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@artokin artokin merged commit 1a9020a into ARMmbed:master Jul 15, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+76 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8549 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@geky

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

Sorry I didn't get a chance to review this, I would general trust @pan- more than me with advanced C++ anyways 😁

Glad these made it in!

I don't know if this is directly related, but someone's broken GitHub's code frequency graph:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.