<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1549/">https://reviewboard.asterisk.org/r/1549/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 31st, 2011, 12:34 p.m., <b>mjordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1549/diff/2/?file=21505#file21505line1548" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/utils.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int add_string_pool(struct ast_string_field_mgr *mgr, struct ast_string_field_pool **pool_head,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1548</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="p">(</span><span class="n">pool</span> <span class="o">=</span> <span class="n">__ast_<span class="hl">c</span>alloc</span><span class="p">(</span><span class="mi"><span class="hl">1</span></span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="n">alloc_size</span><span class="p">,</span> <span class="n">file</span><span class="p">,</span> <span class="n">lineno</span><span class="p">,</span> <span class="n">func</span><span class="p">)))</span> <span class="p">{</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1548</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="p">(</span><span class="n">pool</span> <span class="o">=</span> <span class="n">__ast_<span class="hl">m</span>alloc</span><span class="p">(</span><span class="n">alloc_size</span><span class="p">,</span> <span class="n">file</span><span class="p">,</span> <span class="n">lineno</span><span class="p">,</span> <span class="n">func</span><span class="p">)))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This actually goes against the recommendations in the coding guidelines (2.17.3) - is there a specific reason to not use ast_calloc?</pre>
</blockquote>
<p>On October 31st, 2011, 12:39 p.m., <b>David Vossel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The memset after the malloc is for a smaller amount of memory than the entire allocation. The change appears safe, but I question why we'd even care about this optimization when it adds more complexity.</pre>
</blockquote>
<p>On November 1st, 2011, 1:18 a.m., <b>wdoekes</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This isn't an increase in complexity, this is a fix. The original author should never have used calloc here. This is a very common pattern.
I don't see how you could not care about the overhead of hundreds of bytes getting nulled for nothing when only a handful (struct ast_string_field_pool) should get nulled. Especially in core functions that were created to reduce load (individual string field mallocs) in the first place.</pre>
</blockquote>
<p>On November 1st, 2011, 9:57 a.m., <b>Terry Wilson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">From the coding guidelines:
When allocating/zeroing memory for a structure, use code like this:
struct foo *tmp;
...
tmp = ast_calloc(1, sizeof(*tmp));
Avoid the combination of ast_malloc() and memset(). Instead, always use ast_calloc(). This will allocate and zero the memory in a single operation. In the case that uninitialized memory is acceptable, there should be a comment in the code that states why this is the case.
Considering that this would be initialization code and not something that would be run in a tight loop, the optimization isn't a particularly big deal. We decided long ago to err on the side of caution to protect against people checking if (!foo->bar) against unitialized data. In this case, I would assume that it would be safe since __ast_string_field_init initializes the strings to empty (if I'm reading it correctly, I haven't looked at the stringfields code much). We just have a strong preference to using calloc as a general rule unless it is in an area where performance is really greatly affected during normal runtime.</pre>
</blockquote>
<p>On November 1st, 2011, 10:08 a.m., <b>Terry Wilson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Re-reading that it sounds like I might be against the optimization when I'm not. I was mostly explaining why we normally prefer calloc. If we are indeed initializing with calloc and re-initializing in the init function, that seems kind of silly. We should add a comment explaining that the malloc/memset is done on purpose as it deviates from our standard allocation procedures.</pre>
</blockquote>
<p>On November 1st, 2011, 10:17 a.m., <b>Tilghman Lesher</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm not against the replacement of code, either, but Walter has a misunderstanding of why string fields were created. It was not intended to reduce load, but to reduce the amount of memory that these core structures took. Originally, these structures were created as a series of static buffers, the result of which was that (for example) channel names were both limited in size for the more extreme users, as well as mostly unused for most users. The compromise was to create stringfields, which compacted the amount of memory used, still allowing longer strings for extreme users, while also ensuring that the number of mallocs did not significantly increase.</pre>
</blockquote>
<p>On November 1st, 2011, 10:31 a.m., <b>David Vossel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In general, unless we can actually quantify the performance gained by moving a calloc to a malloc and using memset I am personally against the change. Here is my reasoning. If the calloc has been used since the beginning of time, we always run a risk that somewhere someone is depending on the memory being initialized simply because it always has been, regardless if it should have been or not.
In this case it looks safe, but I am not familiar enough with stringfields to say it is 100% safe without auditing all the stringfield code. Maybe someone else is that familiar with the code and will give it a ship it. Regardless it runs the risk of regression with practically no benefit.
Don't take this to mean I am against code optimization. If this code was re-initializing hundreds of bytes used as the mixing buffer for a conference every 20 ms, this would run a similar risk of regression but the performance benefit would be worthwhile in my opinion.
If you go auditing the code for improvements, you will find them. I never want to discourage that. If someone knows the stringfield code well enough to give this a ship it, then go for it. I just want you to understand the risk of changing things as simple as memory initialization in a codebase as complex as ours.</pre>
</blockquote>
<p>On November 1st, 2011, 10:35 a.m., <b>David Vossel</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looking at my first statement in the previous post, moving a calloc to a malloc with memset will not cause a regression. I was just referring to the change at hand were we change the calloc to use malloc and used memset on a subset of the allocated space for anyone who didn't look at the code in question.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> If the calloc has been used since the beginning of time, we always
> run a risk that somewhere someone is depending on the memory being
> initialized simply because it always has been, regardless if it
> should have been or not.
[...]
> Regardless it runs the risk of regression with practically no
> benefit.
That's reasoning I can dig.
Can I get a "Ship it" without the memset bit? It's completely unrelated to the problem this patch is fixing anyway.</pre>
<br />
<p>- wdoekes</p>
<br />
<p>On October 31st, 2011, 3:47 a.m., wdoekes wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By wdoekes.</div>
<p style="color: grey;"><i>Updated Oct. 31, 2011, 3:47 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This patch fixes that Asterisk can be properly built on certain architectures that dislike misalignment. (In the case of the bug reporter, an ARM.)
==Background==
Currently the 16bit ast_string_field_allocation used in the is not aligned, it can be stored on an 8bit boundary. Certain machines will either SIGBUS over this or simply give wrong results. For the Sparc an #ifdef was added to alleviate the problem.
==Problems with current approach==
(1) The x86 can cope with misaligned integers, but for performance, aligned ints are better.
(2) The #ifdef did not catch all architectures that dislike misalignment.
(3) The code in the #ifdef falsely assumes that the ast_string_field_allocation is at most 2 bytes large. If this were to change one day, things would start to fail again.
==Possible fixes==
(1) Remove the #ifdef, always run the Sparc code and patch it to cope with larger than 16bit ast_string_field_allocation's.
(2) Alter all ast_string_field_allocation code to ensure that base and used stay aligned. Then we won't need to check and re-align later on.
I chose fix #2 because I believe this to be marginally faster and more logical. This does involve the use of the gcc __attribute__((aligned)). But the other code is full of gcc attributes, so I don't think I'm breaking a build anywhere with this.
Regards,
Walter</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I replaced:
typedef uint16_t ast_string_field_allocation;
with:
typedef uint64_t ast_string_field_allocation;
Then I looked at a small sample of base and used during operation.
They were always 64bit aligned.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-17310">ASTERISK-17310</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/include/asterisk/utils.h <span style="color: grey">(342659)</span></li>
<li>/branches/1.8/main/utils.c <span style="color: grey">(342659)</span></li>
<li>/branches/1.8/include/asterisk/stringfields.h <span style="color: grey">(342659)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1549/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>