> On Feb. 2, 2011, 6:30 p.m., Merov Linden wrote: > > indra/llmessage/llpumpio.cpp, line 359 > > <http://codereview.secondlife.com/r/99/diff/2/?file=706#file706line359> > > > > An example where the use of operator() is particularly unsightly... > > Aleric Inglewood wrote: > 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. >
Then it should maybe be made even more obvious with a super-ugly name like getAPRPool_DO_NOT_CALL_THIS_DIRECTLY_use_LLAPRPool_instead() - Boroondas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/99/#review315 ----------------------------------------------------------- On Jan. 29, 2011, 9:10 a.m., Aleric Inglewood wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/99/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2011, 9:10 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > 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). > > > This addresses bug STORM-864. > http://jira.secondlife.com/browse/STORM-864 > > > Diffs > ----- > > doc/contributions.txt fe7fe04ccc9a > indra/llaudio/llaudioengine_fmod.cpp fe7fe04ccc9a > indra/llaudio/llvorbisencode.cpp fe7fe04ccc9a > indra/llcharacter/llbvhloader.cpp fe7fe04ccc9a > indra/llcharacter/llkeyframemotionparam.cpp fe7fe04ccc9a > indra/llcharacter/llstatemachine.cpp fe7fe04ccc9a > indra/llcommon/CMakeLists.txt fe7fe04ccc9a > indra/llcommon/llapp.cpp fe7fe04ccc9a > indra/llcommon/llapr.h fe7fe04ccc9a > indra/llcommon/llapr.cpp fe7fe04ccc9a > indra/llcommon/llaprpool.h PRE-CREATION > indra/llcommon/llaprpool.cpp PRE-CREATION > indra/llcommon/llcommon.h fe7fe04ccc9a > indra/llcommon/llcommon.cpp fe7fe04ccc9a > indra/llcommon/llerror.h fe7fe04ccc9a > indra/llcommon/llerror.cpp fe7fe04ccc9a > indra/llcommon/llfixedbuffer.cpp fe7fe04ccc9a > indra/llcommon/llscopedvolatileaprpool.h PRE-CREATION > indra/llcommon/llthread.h fe7fe04ccc9a > indra/llcommon/llthread.cpp fe7fe04ccc9a > indra/llcommon/llthreadsafequeue.h fe7fe04ccc9a > indra/llcommon/llthreadsafequeue.cpp fe7fe04ccc9a > indra/llcommon/llworkerthread.h fe7fe04ccc9a > indra/llcommon/llworkerthread.cpp fe7fe04ccc9a > indra/llcrashlogger/llcrashlogger.cpp fe7fe04ccc9a > indra/llimage/llimage.cpp fe7fe04ccc9a > indra/llimage/llimagedimensionsinfo.cpp fe7fe04ccc9a > indra/llimage/llimagej2c.cpp fe7fe04ccc9a > indra/llimage/llimageworker.h fe7fe04ccc9a > indra/llimage/llimageworker.cpp fe7fe04ccc9a > indra/llmath/llvolumemgr.cpp fe7fe04ccc9a > indra/llmessage/llares.cpp fe7fe04ccc9a > indra/llmessage/llcurl.cpp fe7fe04ccc9a > indra/llmessage/lliohttpserver.h fe7fe04ccc9a > indra/llmessage/lliohttpserver.cpp fe7fe04ccc9a > indra/llmessage/lliosocket.h fe7fe04ccc9a > indra/llmessage/lliosocket.cpp fe7fe04ccc9a > indra/llmessage/llmail.h fe7fe04ccc9a > indra/llmessage/llmail.cpp fe7fe04ccc9a > indra/llmessage/llpumpio.h fe7fe04ccc9a > indra/llmessage/llpumpio.cpp fe7fe04ccc9a > indra/llmessage/llurlrequest.cpp fe7fe04ccc9a > indra/llmessage/message.cpp fe7fe04ccc9a > indra/llmessage/tests/networkio.h fe7fe04ccc9a > indra/llplugin/llplugininstance.h fe7fe04ccc9a > indra/llplugin/llplugininstance.cpp fe7fe04ccc9a > indra/llplugin/llpluginmessagepipe.cpp fe7fe04ccc9a > indra/llplugin/llpluginprocesschild.cpp fe7fe04ccc9a > indra/llplugin/llpluginprocessparent.h fe7fe04ccc9a > indra/llplugin/llpluginprocessparent.cpp fe7fe04ccc9a > indra/llplugin/llpluginsharedmemory.h fe7fe04ccc9a > indra/llplugin/llpluginsharedmemory.cpp fe7fe04ccc9a > indra/llplugin/slplugin/slplugin.cpp fe7fe04ccc9a > indra/llvfs/lllfsthread.cpp fe7fe04ccc9a > indra/llvfs/llvfs.cpp fe7fe04ccc9a > indra/media_plugins/gstreamer010/llmediaimplgstreamer.h fe7fe04ccc9a > indra/media_plugins/gstreamer010/llmediaimplgstreamer_syms.cpp fe7fe04ccc9a > indra/media_plugins/webkit/linux_volume_catcher.cpp fe7fe04ccc9a > indra/newview/llappviewer.h fe7fe04ccc9a > indra/newview/llappviewer.cpp fe7fe04ccc9a > indra/newview/llappviewerlinux.cpp fe7fe04ccc9a > indra/newview/llappviewerlinux_api_dbus.cpp fe7fe04ccc9a > indra/newview/llappviewermacosx.cpp fe7fe04ccc9a > indra/newview/llfloateranimpreview.cpp fe7fe04ccc9a > indra/newview/llmainlooprepeater.cpp fe7fe04ccc9a > indra/newview/lltexturecache.h fe7fe04ccc9a > indra/newview/lltexturecache.cpp fe7fe04ccc9a > indra/newview/lltexturefetch.cpp fe7fe04ccc9a > indra/newview/llviewermenufile.cpp fe7fe04ccc9a > indra/newview/llvoavatar.cpp fe7fe04ccc9a > indra/newview/llvocache.h fe7fe04ccc9a > indra/newview/llvocache.cpp fe7fe04ccc9a > indra/newview/llvoicevivox.cpp fe7fe04ccc9a > indra/newview/llwatchdog.cpp fe7fe04ccc9a > indra/newview/tests/llworldmap_test.cpp fe7fe04ccc9a > indra/test/lltemplatemessagebuilder_tut.cpp fe7fe04ccc9a > indra/test/message_tut.cpp fe7fe04ccc9a > indra/test/test.cpp fe7fe04ccc9a > indra/test_apps/llplugintest/llmediaplugintest.cpp fe7fe04ccc9a > indra/viewer_components/updater/llupdateinstaller.cpp fe7fe04ccc9a > > Diff: http://codereview.secondlife.com/r/99/diff > > > Testing > ------- > > 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. > > > Thanks, > > Aleric > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
