STORM-864: As as developer, I would like an object oriented wrapper to make safe use of memory pools easier


Submitter:	Aleric Inglewood	Reviewers
Branch:		Groups:	viewer
Bugs:	STORM-864	People:
Change Number:	None	Repository:	viewer-development

Description:

Please see http://jira.secondlife.com/browse/STORM-864

I made a mercurial repository available for testing here:

hg clone https://bitbucket.org/aleric/viewer-development-storm-864

From the commit message:

Introduces a LLThreadLocalData class that can be
accessed through the static LLThread::tldata().
Currently this object contains two (public) thread-local
objects: a LLAPRRootPool and a LLVolatileAPRPool.

The first is the general memory pool used by this thread
(and this thread alone), while the second is intended
for short lived memory allocations (needed for APR).
The advantages of not mixing those two is that the latter
is used most frequently, and as a result of it's nature
can be destroyed and reconstructed on a "regular" basis.

This patch adds LLAPRPool (completely replacing the old one),
which is a wrapper around apr_pool_t* and has complete
thread-safity checking.

Whenever an apr call requires memory for some resource,
a memory pool in the form of an LLAPRPool object can
be created with the same life-time as this resource;
assuring clean up of the memory no sooner, but also
not much later than the life-time of the resource
that needs the memory.

Many, many function calls and constructors had the
pool parameter simply removed (it is no longer the
concern of the developer, if you don't write code
that actually does an libapr call then you are no
longer bothered with memory pools at all).

However, I kept the notion of short-lived and
long-lived allocations alive (see my remark in
the jira here: https://jira.secondlife.com/browse/STORM-864?focusedCommentId=235356&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-235356
which requires that the LLAPRFile API needs
to allow the user to specify how long they
think a file will stay open. By choosing
'short_lived' as default for the constructor
that immediately opens a file, the number of
instances where this needs to be specified is
drastically reduced however (obviously, any
automatic LLAPRFile is short lived).

Testing Done:

Compiles and viewer functions normally.

The new classes (LLAPRPool, LLThreadLocalData) and the LLAPRFile changes have been tested a lot more extensive, since they have been used as-is for months in imprudence and before that in my own viewer (since April). However, the patch contains changes to code elsewhere (to adapt it to the new API) that is rather new in this source tree. Note that changes with regard to LLAPRFile already have been in Snowglobe since version 1.1 (with some minor changes) including the method used to make thread-local data available.

Expand All

I don't feel really qualified to review this, thus just some minor remarks from me:

doc/contributions.txt (Diff revision 1)

This sorting of contribution entries is not related the issue at hand and should thus happen in a separate commit, if at all. Current policy is to not re-sort existing entries at all, to not mislead Oz' metric gathering tools.

See Oz' comment on https://codereview.secondlife.com/r/78/ and his clarification in this thread:
https://lists.secondlife.com/pipermail/opensource-dev/2011-January/thread.html#5329

I guess this policy might change again once the tools have been made more robust. Please postpone re-sorting jira entries until then.

Aleric Inglewood Jan. 28, 2011, 10:41 a.m. (Jan. 28, 2011, 10:41 a.m.)

In the meantime Oz announced that he improved his script so this shouldn't be an issue anymore.
Also, you and me are working on https://github.com/AlericInglewood/contribmerge which will
address this issue once and for all ;) (which was actually triggered by this comment).

indra/llcommon/llaprpool.h (Diff revision 1)

I think this comment line is noteworthy enough to show it on doxygen, too. Maybe even within an \attention or \warning .

Aleric Inglewood Jan. 28, 2011, 10:57 a.m. (Jan. 28, 2011, 10:57 a.m.)

I was under the impression that,

//! Short comment
//
// Still part of doxygen comment.

But you are right, this is not the case.
I will change top level (doxygen) comments to

/** Short comment
 *
 * Still part of doxygen comment.
 */

where I didn't already, which appears to
be a frequently used format in the viewer
code, and for inline multi-line comments,

/// This is a comment
/// of two lines or so.

As seems to be the other choice of
doxygen format used in the viewer code
elsewhere.

indra/llcommon/llaprpool.h (Diff revision 1)

Parameters and their behavior can be documented in doxygen with \param .

See http://www.doxygen.nl/commands.html#cmdparam

Aleric Inglewood Jan. 28, 2011, 11:09 a.m. (Jan. 28, 2011, 11:09 a.m.)

Yes I know... but this isn't really a general comment about the meaning of the parameter, but about the meaning of the default parameter (which looks rather strange). I decided not to write a full-blown doxygen API for this class, since that is done nowhere. Most code doesn't have any documentation, let alone doxygen documentation, and the rest of this class doesn't describe each parameter either. On top of that, the choice of variable names is rather self-explanatory...)

indra/llcommon/llaprpool.h (Diff revision 1)

This could be made a doxygen comment.

Aleric Inglewood Jan. 28, 2011, 11:10 a.m. (Jan. 28, 2011, 11:10 a.m.)
```
Done.
```

indra/llcommon/llaprpool.h (Diff revision 1)

This should probably be a doxygen comment.

Aleric Inglewood Jan. 28, 2011, 11:13 a.m. (Jan. 28, 2011, 11:13 a.m.)

I deliberately didn't make that a doxygen comment because, although it has to be public, it's shouldn't be used elsewhere in the code (except where I already do): it has therefore no importance for anyone to know it exists, or what it does.

indra/llcommon/llaprpool.h (Diff revision 1)

Why include only the first line in doxygen?

Aleric Inglewood Jan. 28, 2011, 11:14 a.m. (Jan. 28, 2011, 11:14 a.m.)
```
Fixed. Now using /** ... */.
```

indra/llcommon/llscopedvolatileaprpool.h (Diff revision 1)

The first "it's" should be "its".

Aleric Inglewood Jan. 28, 2011, 11:14 a.m. (Jan. 28, 2011, 11:14 a.m.)
```
Fixed.
```

indra/llcommon/llscopedvolatileaprpool.h (Diff revision 1)

Consider exposing this comment via doxygen.

Aleric Inglewood Jan. 28, 2011, 11:15 a.m. (Jan. 28, 2011, 11:15 a.m.)
```
Included with the comment above.
```

indra/llcommon/llscopedvolatileaprpool.h (Diff revision 1)

Another candidate for a \warning or \attention ?

Aleric Inglewood Jan. 28, 2011, 11:21 a.m. (Jan. 28, 2011, 11:21 a.m.)
```
Okay, added @attention (I prefer the use of @).
```

indra/llcommon/llthread.h (Diff revision 1)

Another candidate for doxygen. \return could be used.

Aleric Inglewood Jan. 28, 2011, 11:23 a.m. (Jan. 28, 2011, 11:23 a.m.)

This whole file has no doxygen documentation at all. I don't think it is warranted as part of this patch to change that. This is just an API comment thus, using normal C++ comments.

indra/llcommon/llthread.h (Diff revision 1)

Consider using one line per statement, here.

indra/llcommon/llthread.h (Diff revision 1)

It looks like the #if MUTEX_DEBUG is almost redundant here. I think it will only have an effect when RELEASE_SHOW_ASSERT is set or SHOW_ASSERT has been manually set. Are you sure you want to suppress this specific assertion in these cases?

Aleric Inglewood Jan. 28, 2011, 11:34 a.m. (Jan. 28, 2011, 11:34 a.m.)

Either way... This assert would be so seriously wrong that I'm assuming it is never triggered and hence a waste of cpu.
I removed the #if MUTEX_DEBUG ... (because of your comment) because that is not really related to this assert: it doesn't really mean "debug mutexes", it means "test if some mutexes are used recursively. Note that this code is normally never used: Linden Lab's libapr is compiled with APR_HAS_THREADS, and a developer should use a libapr library compiled like that too, anyway.

indra/llcommon/llthread.cpp (Diff revision 1)

Worth inlining?

Aleric Inglewood Jan. 28, 2011, 11:40 a.m. (Jan. 28, 2011, 11:40 a.m.)

I don't think so. First of all, it's only used for the debug version of a viewer where performance (at THIS level) is not an issue, secondly, it has to be available everywhere so inlining would require main_thread_id to be global and exported from libcommon (instead) and libapr headers to be included for every .cpp file (not sure, but I don't think they are currently).

Diff
- added Diff r2

Description:

This new diff incorporates the changes as discussed above, and is updated to be based on revision fe7fe04ccc9a (Fri Jan 28 21:23:35 2011 -0700).
I also added comments in this version on every line that correctly uses apr_pool_t, so it's easy to just do a grep on apr_pool_t and check if nobody accidently used that instead of LLAPRPool (in the future).

I made this patch (again) available as mercurial repository, here:

hg clone https://bitbucket.org/aleric/viewer-development-storm-864

(that is the same name as before, but be warned that I destroyed
the old one and created a new repository).

First pass and I couldn't see anything wrong with the new structure. It simplifies the code using APR quite a bit and surely makes it more resilient. I'll do a pull and build on a test repo before giving my stamp of approval. It also surely needs to be reviewed by Bao who dealt with that code quite a bit and might have more pointed comments.

indra/llcommon/llapr.cpp (Diff revision 2)

Use "its reference count"

indra/llcommon/llaprpool.h (Diff revision 2)

Ooops! Wrong license. We now use LGPL. Please shange to "license=viewerlgpl"

indra/llcommon/llaprpool.h (Diff revision 2)

The rest of the license should be changed too. Please take example on any currently shipped file for an appropriate header.

indra/llcommon/llaprpool.h (Diff revision 2)

Use "its subpools"

indra/llcommon/llaprpool.h (Diff revision 2)

Use "its parents"

indra/llcommon/llaprpool.cpp (Diff revision 2)

Same comment wrt license as before: use LGPL.

indra/llcommon/llaprpool.cpp (Diff revision 2)

K,it's just a paranoid assert. Still, I'm curious how you came up with the (*4) value. Also, not much point using bitshift here (I know: old habits... :)

Aleric Inglewood Feb. 3, 2011, 7:05 p.m. (Feb. 3, 2011, 7:05 p.m.)

I have no idea :p. This assert wasn't created by me, it comes from the old code.

indra/llcommon/llscopedvolatileaprpool.h (Diff revision 2)

Same comment on license: use LGPL

indra/llcommon/llaprpool.h (Diff revision 2)

I think it's be more clear to use an explicit name like "getAPRPool()" rather than the function operator here. It produces hard to read code in a couple of places and it's just plain unclear what "myPool()" really does.

Aleric Inglewood Feb. 3, 2011, 7:09 p.m. (Feb. 3, 2011, 7:09 p.m.)

That is actually on purpose. Developers should never actually "get" the pool.
Like the comment says, it's painful that we have to provide access at all:
we want to hide apt_pool_t completely from the user. The user should use
LLAPRPool instead. And then, when that has to be passed to the actual
libapr function, it's passed like this:  apr_whatever_create(foo, mPool());
because that is just a little bit more safe than have it auto convert
and allow writing apr_whatever_create(foo, mPool); or so I thought.
It's deliberately a bit weird, to make people think twice before they
"get" the underlaying apr_pool_t.  Also... see comment below...

indra/llmessage/llpumpio.cpp (Diff revision 2)

An example where the use of operator() is particularly unsightly...

Aleric Inglewood Feb. 3, 2011, 7:26 p.m. (Feb. 3, 2011, 7:26 p.m.)

And rightfully so! It should be "extremely unpleasant" for the user to get to the underlaying apr_pool_t*.
That this code hacks access is exactly that: a hack. One has to be very careful when doing this.
The reason I did it here is because 1) I know what I'm doing, so it's ok in this case, 2) for this
first introduction of LLAPRPool I tried to make minimal changes to the actual functionality of
existing code using apr pools (with the exception of LLAPRFile, the rewrite of its API actually
coming from SNOW-103 which already proved itself in snowglobe, imprudence and other TPV's).
The fact that this access here is needed actually signifies that this code is not handling
pools in a very safe way, but I consider(ed) it better to keep the code "as-is" and hack around the
*safity* of LLAPRPool (and as a result not changing anything!) than to rewrite the interface at this
point for this particular part of the code; changing things is more risk, in this case, than not
using the API of LLAPRPool as intended. It would be harder to find if that rewrite would introduce
some kind of bug we made lots of changes at the same time. I propose to leave this as it is now
and look at changing it later once everyone feels secure about the stability of the current patch.

Okay, that sounded nice -- but the truth is that I have no idea (at this point) if it is even
possible to do what that code does without accessing the underlaying apr_pool_t* like that (meaning,
not passing it directly to an APR function, but storing it in a structure). I just know that it
should feel dangerous to do so, so it seems OK that code that does it doesn't look very nice.

Boroondas Gupte Feb. 4, 2011, 3:53 a.m. (Feb. 4, 2011, 3:53 a.m.)

Then it should maybe be made even more obvious with a super-ugly name like getAPRPool_DO_NOT_CALL_THIS_DIRECTLY_use_LLAPRPool_instead()

Aleric Inglewood Feb. 4, 2011, 6:24 a.m. (Feb. 4, 2011, 6:24 a.m.)

That is not possible, as operator() is definitely part of the normal API (it's just unfortunate that it has to exist).
Let me explain more about what is so "dangerous" about allowing access. Also, realize that before this patch
everything was completely open (accessible) and spread out through the code, so it's at least 1000 times better
this way -- just not 100% bullet proof).

Firstly, we can't use getAPRPool_DO_NOT_CALL_THIS_DIRECTLY_use_LLAPRPool_instead() instead of
operator()(void), because that would change perfectly legal code like (where mPool is a LLAPRPool):

     apr_thread_cond_create(&mAPRCondp, mPool());

into

     apr_thread_cond_create(&mAPRCondp, mPool.getAPRPool_DO_NOT_CALL_THIS_DIRECTLY_use_LLAPRPool_instead());

where mPool() is the same as mPool.operator()(), returning the underlaying apr_pool_t* to pass it to
the apr library call using it.

The reason that I call it "dangerous" that the user (that is, other developers that use this
LLAPRPool interface) can access the underlaying apr_pool_t* is because they might be tempted
to do something like:

     apr_pool_t* pool = mPool();
     // Use 'pool'.
     apr_pool_destroy(pool);

Fortunately, this would be easy detectable by grepping the source code for 'apr_pool_t'.
Less obvious would be:

     some_apr_defined_struct.pool = mPool();
     // leave some_apr_defined_struct around for OTHER threads to use.

That would, obviously, defy the thread-safity checks of LLAPRPool, and we also wouldn't
find it by grepping for apr_pool_t because this struct is defined in a header of libapr.

I'd almost say that one could call it "far fetched" to assume that anyone would
do anything like this-- so the chance that anything wrong ever happening is indeed
very small. Nevertheless, to avoid all that, I made the 'rule of thumb' that the
operator() only be used WHEN passing the pool directly to an apr_* library call.

It would have been better if we could even drop that restriction (by getting rid of
the current possibility to corrupt things by getting access to the underlaying
apr_pool_t), but that is only possible if the operator() is removed completely
and we overload all apr_* functions that we use (making those friend of LLAPRPool,
so they can call the real libapr function with apr_pool_t* parameter). I didn't feel
that the remaining security risk warranted that though.

Nevertheless, if merov wants then I can do this. The only draw back would
be that 1) every source file would need to include the apr_pools.h (through
linden_common.h I presume), just to stop every source code from possibly
including that themselves and using apr pools directly. 2) Every time we want
to use a new APR library function that needs a pool to be passed, we'd have
to add it to the list of wrappers.

Actually, both are pretty minimal changes; I'd have no objection or problem
with this change at all (I just thought it would be harder to accept).

For the rest of the code this would result in 1) code like,

     apr_thread_cond_create(&mAPRCondp, mPool);  // No more '()'

while 2) code like,

     some_apr_defined_struct.pool = mPool; // or mPool()

wouldn't compile anymore.

Oz Linden Feb. 4, 2011, 10:01 a.m. (Feb. 4, 2011, 10:01 a.m.)

I'm going to weigh in on the side of providing a getPool() method rather than overloading the function call operator.

I get your point about the danger, and perhaps we should look at the cases in the existing code where it is needed to see if perhaps additional methods (static wrappers?) would help ameliorate the risk.

Operator overloading should be used only when it is the most natural solution (as in using + for string concatenation); in any other case it just obscures what is going on, and I don't think that it's a good way of dealing with the risk you are concerned about.

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.