Summary:	ASTERISK-03494: [PATCH] rework channel structure to save more memory
Reporter:	Kevin P. Fleming (kpfleming)	Labels:
Date Opened:	2005-02-13 00:30:22.000-0600	Date Closed:	2008-01-15 15:27:06.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) channel_tech_rev3.diff.txt
Description:	The attached patch is large and invasive. Essentially, it does these things: - eliminate the channel_pvt structure, in favor of the channel module providing a method table for its channels - allow the channel to store its private data directly off the ast_channel structure, instead of off the channel_pvt Good news: - on a 32-bit system, the channel memory usage goes down 68 bytes (plus the size of a malloc block header) and requires one less malloc (2 instead of 3)... on a 64-bit system, it's 144 bytes plus the malloc block header - access to channel private data now takes one less pointer dereference (which happens on every call into the channel's methods) - channel setup now only requires copying one pointer, instead of anywhere from 6 to 22 depending on the channel Bad news: - out-of-tree channel modules will no longer compile; the changes required are pretty straightforward (at least half are just search-and-replace) I have compiled and tested chan_sip, chan_iax2, chan_zap and chan_features. I have compiled (but not tested) chan_agent, chan_local, chan_mgcp, chan_modem, chan_phone and chan_skinny. I have _not_ compiled chan_alsa, chan_vpb, chan_nbs or chan_h323, since my system does not have the required libraries/headers. This needs lot of testing to ensure I didn't make any copy-and-paste errors, but the structures and memory management seem sound. I'll post on -dev to get the attention of the out-of-tree channel maintainers. ****** ADDITIONAL INFORMATION ****** Disclaimer is on file.
Comments:	By: Kevin P. Fleming (kpfleming) 2005-02-13 00:44:29.000-0600 More notes: ast_channel_alloc has been moved to channel_old.c and channel_old.h, and marked as deprecated (and will generate a single warning on use during an Asterisk process lifetime). ast_channel_alloc_tech is the replacement, which assigns the channel's technology when the channel structure is created. channel_pvt.h is gone, but everything it used to provide (_except_ the channel_pvt structure itself) has been moved to channel.h. If this patch is merged, the CVS repository will need to have channel_pvt.h manually removed. These should be the only changes visible to any modules that are not channel drivers themselves; all in-tree users of this function and include file have been updated. edited on: 02-13-05 00:45 By: Mark Spencer (markster) 2005-02-13 00:59:36.000-0600 This is certainly an interesting idea, but I have a few comments: 1) I see really no reason to change ast_channel_alloc.  There are still so many things to be set (e.g. name, etc) that I see no reason to pass the technology to it. 2) Clearly ast_do_masquerade is *far* from being accurate.  Since you are having to replace the code which simply moves the pvt portion of the channel, you clearly will have to actually move now *all* the fields that were at one point contained there, including rawreadformat and rawriteformat. I'll try to put more review into it as well after a rev. By: Kevin P. Fleming (kpfleming) 2005-02-13 01:08:40.000-0600 For ast_do_masquerde, the raw formats are already being swapped by code that this patch does not touch. I do not swap the translations, because they are closed and freed before the pvt structure is swapped. In other words, I already did spend the time to look at every single field in ->pvt to determine whether it needed to still be swapped or not. I may have missed something, but I think I covered them all. Yes, it would be possible to not change the prototype for ast_channel_alloc, but there are code paths that assume that ->tech will not be NULL, ast_channel_alloc will just have to set it to &null_tech, then the channel driver can override it later. That should be safe. By: Kevin P. Fleming (kpfleming) 2005-02-13 01:10:56.000-0600 Yeah... you're right, I missed the rawformat fields. I confused them with the regular format fields, which are already handled. I'll fix that up. By: Kevin P. Fleming (kpfleming) 2005-02-13 01:55:38.000-0600 OK, rev2 reverts the ast_channel_alloc_tech change, and ensures that ast_do_masquerade swaps rawformats as well. By: Mark Spencer (markster) 2005-02-28 00:28:25.000-0600 So is this (subject to being updated for latest CVS) ready for merge? By: Kevin P. Fleming (kpfleming) 2005-02-28 00:34:01.000-0600 Yes, it's been working flawlessly here, although I've not updated it to CVS in a while (obviously). I can do so tomorrow evening or Tuesday morning. By: Kevin P. Fleming (kpfleming) 2005-03-03 20:23:45.000-0600 OK, rev3 uploaded, updated to match current CVS HEAD and tested here for SIP and IAX2. By: Mark Spencer (markster) 2005-03-04 00:49:56.000-0600 *holds breath* okay, here goes!  (a.k.a. added to CVS with mods to allow chan_alsa and chan_nbs to build again)... By: Digium Subversion (svnbot) 2008-01-15 15:26:55.000-0600 Repository: asterisk Revision: 5137 U   trunk/.cleancount U   trunk/apps/app_math.c U   trunk/apps/app_parkandannounce.c U   trunk/apps/app_sendtext.c U   trunk/apps/app_voicemail.c U   trunk/autoservice.c U   trunk/channel.c U   trunk/channels/chan_agent.c U   trunk/channels/chan_alsa.c U   trunk/channels/chan_features.c U   trunk/channels/chan_h323.c U   trunk/channels/chan_iax2.c U   trunk/channels/chan_local.c U   trunk/channels/chan_mgcp.c U   trunk/channels/chan_modem.c U   trunk/channels/chan_nbs.c U   trunk/channels/chan_oss.c U   trunk/channels/chan_phone.c U   trunk/channels/chan_sip.c U   trunk/channels/chan_skinny.c U   trunk/channels/chan_vpb.c U   trunk/channels/chan_zap.c U   trunk/cli.c U   trunk/dsp.c U   trunk/image.c U   trunk/include/asterisk/channel.h D   trunk/include/asterisk/channel_pvt.h U   trunk/include/asterisk/rtp.h U   trunk/include/asterisk/vmodem.h U   trunk/pbx/pbx_dundi.c U   trunk/pbx.c U   trunk/res/res_features.c U   trunk/res/res_musiconhold.c U   trunk/rtp.c U   trunk/translate.c ------------------------------------------------------------------------ r5137 | markster | 2008-01-15 15:26:54 -0600 (Tue, 15 Jan 2008) | 2 lines Rework channel structure to eliminate "pvt" portion of channel (bug ASTERISK-3494) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=5137 By: Digium Subversion (svnbot) 2008-01-15 15:27:06.000-0600 Repository: asterisk Revision: 5151 U   trunk/channels/chan_vpb.c ------------------------------------------------------------------------ r5151 | bkramer | 2008-01-15 15:27:06 -0600 (Tue, 15 Jan 2008) | 2 lines / updated to work with new fixes for bug ASTERISK-3494 ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=5151