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-0600Date Closed:2008-01-15 15:27:06.000-0600
Versions:Frequency of
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.


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)



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