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 the memory consumed by FunctionPointerWithContext instances #81

Merged
merged 3 commits into from
Oct 30, 2015

Conversation

pan-
Copy link
Member

@pan- pan- commented Oct 23, 2015

This patch reduce the memory consumed by FunctionPointerWithContext instances.

The original size of a FunctionPointerWithContext is 32 bytes, with this patch it goes down to 20bytes. With this change, it is possible to save 24 bytes by GattCharacteristic.

The idea is simple, instead of storing a function pointer, an object and a pointer to a member function it only store the function pointer OR the object and the pointer to the member function.

An undefined pointer to member function type is used to calculate the true size of the biggest possible pointer to member function and align properly the pointer to member stored.

bytes (oryginally, it was 32 bytes !).
Enforce alignement constraints of the embedded pointer to member function.
Symplify and unify call mecanic, everything is delegated uniformally to the actual
implementation.
@rgrover
Copy link
Contributor

rgrover commented Oct 23, 2015

FYI: There also exists https://github.com/ARMmbed/core-util/blob/master/core-util/FunctionPointer.h, but 'ble' needed a FunctionPointer even before the version in core-utils was born. We can't re-use the version in core-utils because for the moment ble needs to remain compatible with mbed-classic.

UndefinedMemberFunction _allignement;
};
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep only a single blank line.

@pan-
Copy link
Member Author

pan- commented Oct 23, 2015

I have removed the unneeded blank lines, really sorry for that.

I know for the "FunctionPointer" in core-utils. I also see some way to improve it :

  • why do we have to rely on different name when the number of arguments change ? If we use partial specialization, it can be easy to have a unique name plus it will look like std::function
FunctionPointer<int()> foo;
FunctionPointer<int(char, int)> bar;

// instead of 

FunctionPointer0<int> foo;
FunctionPointer2<int, char, int)> bar;
  • We don't support function object right now, this will be problematic once we will enable c++11 support. Users will be limited when the will use lambda. This can be improved
  • It does not support value semantic or other kind of pointer like SharedPtr for the pointer to the instance. This can be added.
  • It still takes a lot of space, I think we can work around this.

MemberFunctionAndPtr _memberFunctionAndPointer;
};

void (*_caller)(FunctionPointerWithContext*, ContextType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we could re-use pFunctionPointerWithContext_t instead of FunctionPointerWithContext* in void (*_caller)(FunctionPointerWithContext*, ContextType); ?

@0xc0170
Copy link

0xc0170 commented Oct 26, 2015

@pan- I like your comments above, please can you create issues in core-utils to track them?

	- add a space after if keyword
	- Use typedef types instead of direct declarations for
	  pFunctionPointerWithContext_t and pvoidfcontext_t
	- Fix typos and enhance comment about how alignement and size
	  requirements of the member function pointer are computed.
@pan-
Copy link
Member Author

pan- commented Oct 26, 2015

I have updated the code regarding your comments.

I have also open a detailed issue in core-utils : ARMmbed/core-util#39

I hope it doesn't sound like a rant, this is not the point.

@0xc0170
Copy link

0xc0170 commented Oct 26, 2015

@pan- Nope, it does not ! Thanks. Any feedback appreciated, even feel free to send PR , as you already implemented some improvements here.

rgrover added a commit that referenced this pull request Oct 30, 2015
Reduce the memory consumed by FunctionPointerWithContext instances
@rgrover rgrover merged commit 99ba55b into ARMmbed:develop Oct 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants