[Asterisk-code-review] astobj2: Reduce memory overhead. (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/10294 )

Change subject: astobj2: Reduce memory overhead.
......................................................................


Patch Set 1: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/10294/1/main/astobj2.c
File main/astobj2.c:

https://gerrit.asterisk.org/#/c/10294/1/main/astobj2.c@50
PS1, Line 50: struct __priv_data {
FYI

I think you are getting an overall reduction in struct size but the amount is dependent upon the architecture because you are shifting padding bytes around and eliminating a uint32_t member.  For the x86_64 you are recovering 4 bytes of padding and eliminating a 4 byte uint32_t for 8 bytes.

8 bytes per pointer
4 bytes per size_t (This may be 8 depending upon the base int type of size_t)
4 bytes per int32_t/uint32_t

This gives this struct
8 + 8 + (4 or 8) + 4 + 4 = 28 or 32 bytes used

The alignment of the struct is going to be based upon the largest alignment object in the struct which in this case would be a pointer.  So the alignment will be 8.

Then taking the alignment of the struct into consideration, there will be 4 bytes of padding at the end of the struct if size_t is 4 bytes.  So the size of the struct should now be 32 regardless of the size_t size.

For the original layout:
4 + 4(pad) + 8 + 8 + (4 or 8) + 4 + 4 = 36 or 40 bytes used

With the overall struct alignment the size would be 40 because there may or may not be 4 bytes of padding at the end of the struct to bring it to 8 byte alignment.


https://gerrit.asterisk.org/#/c/10294/1/main/astobj2.c@60
PS1, Line 60: 	/*! The ao2 object option flags */
            : 	uint32_t options:2;
            : 	/*! magic number.  This is used to verify that a pointer passed in is a
            : 	 *  valid astobj2 or ao2_weak reference */
            : 	uint32_t magic:30;
FYI

You are squeezing the bits out of the magic number safety/sanity check.  We now have 29 bits out of 32 for the safety check.

I suppose if we ever need any more bits for the ao2 options flags we will have to pull options back out as its own uint32_t.


https://gerrit.asterisk.org/#/c/10294/1/main/astobj2.c@545
PS1, Line 545: 				current_value, user_data);
To be safe you should cast current_value to int here.


https://gerrit.asterisk.org/#/c/10294/1/main/astobj2.c@554
PS1, Line 554: 				file, line, func, ret, tag);
To be safe you should cast ret here to int.


https://gerrit.asterisk.org/#/c/10294/1/main/astobj2.c@563
PS1, Line 563: 			"Invalid refcount %d on ao2 object %p\n", current_value, user_data);
To be safe you should cast current_value to int here.


https://gerrit.asterisk.org/#/c/10294/1/main/astobj2.c@696
PS1, Line 696: 	obj->priv_data.ref_counter = 1;
             : 	obj->priv_data.destructor_fn = destructor_fn;	/* can be NULL */
             : 	obj->priv_data.options = options;
             : 	obj->priv_data.magic = AO2_MAGIC;
Should reorder this to match the new layout.



-- 
To view, visit https://gerrit.asterisk.org/10294
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idc1baabb35ec3b3d8de463c4fa3011eaf7fcafb5
Gerrit-Change-Number: 10294
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 17:05:27 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180927/82b3bcf0/attachment.html>