Callback extension and optimisation #12036

kjbracey · 2019-12-05T14:10:12Z

Summary of changes

Extend Callback, and optimise it.

Impact of changes

Optimise clearing by adding nullptr overload.
Optimise clearing by not clearing storage - increases code size of comparison, but that is extremely rare.
Reduce ROM used by trivial functors - share copy/destroy code.
Config option to force trivial functors - major ROM saving by eliminating the "operations" table.
Config option to eliminate comparison altogether - minor ROM saving by eliminating zero padding.
Conform more to std::function API.
~~Force trivial functors by default to get major ROM saving.~~ (now in Callback: Trivial default #12761)

Migration actions required

The added nullptr overload means Callback(NULL) or Callback(0) will no longer work; users must use Callback(nullptr) or Callback().
~~If application code uses Callback with a non-trivial functor, they will get a compilation error directing them to turn on platform.callback-nontrivial.~~

Documentation

nullptr: Callback: nullptr adjustments mbed-os-5-docs#1184
Config: Add docs for Callback config mbed-os-5-docs#1185

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[X] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers

@pan-

kjbracey · 2019-12-05T14:11:26Z

This PR is based on and includes #12035, which deals with the breaking changes. Last 3 commits are the new content.

ciarmcom · 2019-12-05T16:00:48Z

@kjbracey-arm, thank you for your changes.
@pan- @ARMmbed/mbed-os-core @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-test @ARMmbed/mbed-os-maintainers please review.

Adjust wording to talk of Callback's being "empty", which is clearer than "null", and avoids thought of `NULL`. Add explicit note about using `nullptr` to reset. Preparation for ARMmbed/mbed-os#12036 - can be merged now as adjustments are valid for existing code.

Documentation for changes in ARMmbed/mbed-os#12036

kjbracey · 2019-12-11T11:44:17Z

I've posted a query about the GCC failure I saw working on this: https://answers.launchpad.net/gcc-arm-embedded/+question/686870

Although the failure has now apparently gone away since removing the full memset of storage, so I'll be removing the workaround of doing memcpy into storage rather than placement-new.

adbridge · 2019-12-23T13:41:30Z

@kjbracey-arm looks like this also needs a rebase now ...

adbridge · 2020-01-06T11:32:41Z

@kjbracey-arm looks like this also needs a rebase now ...

@kjbracey-arm bump....

0Grit · 2020-01-06T21:45:40Z

@AGlass0fMilk please review

* Optimise clearing by adding `nullptr` overload. This overload means `Callback(NULL)` or `Callback(0)` will no longer work; users must use `Callback(nullptr)` or `Callback()`. * Optimise clearing by not clearing storage - increases code size of comparison, but that is extremely rare. * Reduce ROM used by trivial functors - share copy/destroy code. * Config option to force trivial functors - major ROM saving by eliminating the "operations" table. * Config option to eliminate comparison altogether - minor ROM saving by eliminating zero padding. * Conform more to `std::function` API.

kjbracey · 2020-03-12T07:36:05Z

Fixed up again - had pushed an incorrect version.

Also fixed some problems I was having with breaking optimisation of the "trivial" mode. Long comment with links to explanations included.

0xc0170 · 2020-03-24T15:36:07Z

CI restarted

mbed-ci · 2020-03-24T21:24:57Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 9
Build artifacts

0xc0170 · 2020-03-25T09:12:31Z

@ARMmbed/mbed-os-core @bulislaw Please review

bulislaw · 2020-03-27T09:34:15Z

features/netsocket/NetworkInterface.h

@@ -352,13 +352,15 @@ class NetworkInterface: public DNS {
     */
    void add_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);

+#if MBED_CONF_PLATFORM_CALLBACK_COMPARABLE
    /** Remove event listener from interface.
     *
     * Remove previously added callback from the handler list.
     *
     *  @param status_cb The callback to unregister.
     */
    void remove_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);


Would be good to mention in doxygen that this function is not available if the callback_comparable flag is false, it may be not obvious if user looks at the doxygen and handbook only.

bulislaw

I don't understand all of this C++ magic, but I trust Vincent's review :) The rest looks generally ok.

* Add docs for Callback config Documentation for changes in ARMmbed/mbed-os#12036 * "left set to false" -> "set to false" Make more ambiguous about default, as there's currently a pending change. Co-Authored-By: Irit Arkin <[email protected]> Co-authored-by: Irit Arkin <[email protected]>

kjbracey added the needs: preceding PR label Dec 5, 2019

kjbracey force-pushed the callback_fiddle branch from c0ac861 to 9bd734f Compare December 5, 2019 14:39

ciarmcom requested review from pan- and a team December 5, 2019 16:00

ciarmcom added the needs: review label Dec 5, 2019

kjbracey force-pushed the callback_fiddle branch from 9bd734f to ae7174b Compare December 9, 2019 08:45

kjbracey added a commit to kjbracey/mbed-os-5-docs that referenced this pull request Dec 9, 2019

Add docs for Callback config

10d94c5

Documentation for changes in ARMmbed/mbed-os#12036

This was referenced Dec 9, 2019

Callback: nullptr adjustments ARMmbed/mbed-os-5-docs#1184

Merged

Add docs for Callback config ARMmbed/mbed-os-5-docs#1185

Merged

kjbracey force-pushed the callback_fiddle branch 3 times, most recently from c0d55f0 to 5e12858 Compare December 10, 2019 08:54

kjbracey force-pushed the callback_fiddle branch 2 times, most recently from 47677c5 to 6adc0a0 Compare December 11, 2019 13:41

adbridge requested a review from bulislaw December 17, 2019 16:53

kjbracey force-pushed the callback_fiddle branch from 6adc0a0 to 3c67014 Compare December 19, 2019 10:32

adbridge added needs: work and removed needs: review labels Dec 23, 2019

kjbracey force-pushed the callback_fiddle branch 2 times, most recently from 623b71d to 587ebc5 Compare March 11, 2020 09:17

mergify bot added needs: work and removed needs: CI labels Mar 11, 2020

kjbracey force-pushed the callback_fiddle branch from 587ebc5 to c996b01 Compare March 11, 2020 13:12

kjbracey added 3 commits March 11, 2020 15:46

Add more Callback tests

171a5f8

Disable test that breaks IAR

d09d854

kjbracey force-pushed the callback_fiddle branch from c996b01 to d09d854 Compare March 11, 2020 13:46

0xc0170 added needs: CI and removed needs: work labels Mar 24, 2020

0xc0170 approved these changes Mar 25, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Mar 25, 2020

0xc0170 added needs: review release-version: 6.0.0-beta-1 and removed ready for merge labels Mar 25, 2020

bulislaw reviewed Mar 27, 2020

View reviewed changes

bulislaw approved these changes Mar 27, 2020

View reviewed changes

adbridge added ready for merge and removed needs: review labels Mar 27, 2020

adbridge merged commit 009ff7a into ARMmbed:master Mar 27, 2020

adbridge removed the ready for merge label Mar 27, 2020

This was referenced Apr 6, 2020

Callback: Trivial default #12761

Merged

Turn on platform.callback-nontrivial ARMmbed/mbed-os-example-filesystem#115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Callback extension and optimisation #12036

Callback extension and optimisation #12036

Uh oh!

kjbracey commented Dec 5, 2019 •

edited

Loading

Uh oh!

kjbracey commented Dec 5, 2019

Uh oh!

ciarmcom commented Dec 5, 2019

Uh oh!

kjbracey commented Dec 11, 2019

Uh oh!

adbridge commented Dec 23, 2019

Uh oh!

adbridge commented Jan 6, 2020

Uh oh!

0Grit commented Jan 6, 2020

Uh oh!

kjbracey commented Mar 12, 2020

Uh oh!

0xc0170 commented Mar 24, 2020

Uh oh!

mbed-ci commented Mar 24, 2020

Uh oh!

0xc0170 commented Mar 25, 2020

Uh oh!

bulislaw Mar 27, 2020

Uh oh!

bulislaw left a comment

Uh oh!

Uh oh!

Callback extension and optimisation #12036

Callback extension and optimisation #12036

Uh oh!

Conversation

kjbracey commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

kjbracey commented Dec 5, 2019

Uh oh!

ciarmcom commented Dec 5, 2019

Uh oh!

kjbracey commented Dec 11, 2019

Uh oh!

adbridge commented Dec 23, 2019

Uh oh!

adbridge commented Jan 6, 2020

Uh oh!

0Grit commented Jan 6, 2020

Uh oh!

kjbracey commented Mar 12, 2020

Uh oh!

0xc0170 commented Mar 24, 2020

Uh oh!

mbed-ci commented Mar 24, 2020

Test run: SUCCESS

Uh oh!

0xc0170 commented Mar 25, 2020

Uh oh!

bulislaw Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kjbracey commented Dec 5, 2019 •

edited

Loading