> On Jan. 20, 2011, 10:57 p.m., Merov Linden wrote: > > I fail to see how any of those changes "massively prevents object > > duplication". I don't disagree with any of them but I don't see much value > > in any either. Sure, there are different ways to skin a cat and some are > > better. Still, these changes will likely make upcoming merges conflict > > annoyingly and make the life of the maintainers (i.e. mine and Oz) horrid. > > It reminded me of this post by another (more famous) Open Source > > maintainer: http://tirania.org/blog/archive/2010/Dec-31.html
Hi Merov! Firstly, I completely agree with the Open Source maintainer article, and I understand why this patch made you think it. However, being aware of this fact I don't think I made such changes to code that didn't already need change anyway. In fact, although certain styles used in the code are a horror in my eyes, I still don't change it-- even when I change those lines to fix a bug-- when the context around it all use the same style. A lot of style in the viewer code changes from file to file and even from function to function, but I try to preserve the style used at that point in the code. But back to the subject at hand. Surely you know most of the following already: I'm not writing this just for you but more in general for anyone reading this review (to produce some of the output below, I had to recompile the viewer several times, so I have some time to write this ;). Perhaps the two of us can talk about it on IRC, as well, if you want. In the line of the article that you linked to ;) ... you might just want to skip this long post to where it say ******START HERE******. An important concept to understand this patch is what is called Plain Old Data (POD). All built-in types are POD, as well as struct's that that only have POD members (in particular, have no user-defined constructor or destructor, for the gory definition read http://en.wikipedia.org/wiki/Plain_old_data_structure) other then the ones generated by the compiler. Why does this matter? Well, consider we have two .cpp source files, source1.cpp and source2.cpp, both of which include the same header: source1.cpp: #include "header.h" //... int main() { } source2.cpp: #include "header.h" //... Further more, assume that the header defines a few constants: header.h: typedef int POD1; struct POD2 { POD1 i1; POD1 i2; }; POD1 const const1 = 1; POD2 const const2 = { 2, 2 }; struct nonPOD { POD1 i; nonPOD(int i_) : i(i_) { } }; nonPOD const const3(3); Here, the struct nonPOD is not a POD type because it has a user defined constructor. Note that 'nonPOD const const3 = { 3 }' would not compile, as such initialization is only allowed for POD types. First we'd compile both source files: g++ -o source1.o -c source1.cpp g++ -o source2.o -c source2.cpp And then link them: g++ -o test source1.o source2.o We can use the utility nm(1) to dump the symbols of each object file: $ nm -C ./source1.o | egrep '(const[0-9]|POD)' 0000000000000000 r const1 0000000000000004 r const2 0000000000000000 b const3 0000000000000000 W nonPOD::nonPOD(int) Here, just showing the interesting stuff (symbols containing 'const' or 'POD'), we see all three constants, and the constructor of nonPOD. Note that const1 and const2 have an 'r' in front of them while const3 has a 'b'. This means respectively 'read-only section' and 'uninitialized section'. The uninitialized section is not part of the final executable, it is space allocated in memory at the moment an executable is started and subsequently initialized, so it doesn't contribute the size of the executable but it does cause more memory to be used during run time. The other object file, source2.o has of course the exact same output: $ nm -C ./source2.o | egrep '(const[0-9]|POD)' 0000000000000000 r const1 0000000000000004 r const2 0000000000000000 b const3 0000000000000000 W nonPOD::nonPOD(int) Finally we can look at the end result, the linked executable: $ nm -C test | egrep '(const[0-9]|POD)' 000000000040071c r const1 0000000000400724 r const1 0000000000400720 r const2 0000000000400728 r const2 0000000000600b10 b const3 0000000000600b14 b const3 00000000004005d2 W nonPOD::nonPOD(int) And as you see all constants appear twice in the executable. Nothing we can do about that (until we compile with optimization, see below). Note that nm uses certain debug information. If we strip the executable, like so; $ strip test $ nm -C test | egrep '(const[0-9]|POD)' nm: test: no symbols So clearly this doesn't give the REAL picture, but good enough for this explanation. If now we compile with optimization, which should be the case that concerns us most, then the results becomes: $ g++ -O -o source1.o -c source1.cpp $ g++ -O -o source2.o -c source2.cpp $ g++ -o test source1.o source2.o 00000000006009f0 b const3 00000000006009f4 b const3 In other words, optimizing is able to get rid of POD constants, but not of non-POD constants: the compiler doesn't see or know that these will become identical (and in fact, if we'd take the pointer of them in yet other source files, then those _should_ be different). This is why it's at least good practise to put non-POD constants NOT in a header file: You don't gain anything with it, except creating duplicates. Now lets have a look at these results for do-not-directly-run-secondlife-bin (RELEASE mode, so with optimization). Here we need to use a more elaborate command line in order to get rid of not so interesting information: We dump all symbols, filter out only the ones in the read-only section and the uninitialized section, strip of all output except the mangled symbol name, then sort the symbol names alphabetically (not removing duplicates) and pipe that into 'uniq -c' which prints each symbol prepended with a count of how often it occurred. That result is piped into sort -n to sort the this result by how often each symbol (name) occurred (largest count last) and subsequencially through c++filt to demangle the symbols. Finally we filter out non interesting symbols: labels (starting with .LC), __FUNCTION__ symbols with the name of functions, the '_site' variable that occurs in the info and warning stream macro's, so although it's name is repeated a lot, it's really a different symbol every time. The result is of that is: $ nm do-not-directly-run-secondlife-bin | egrep ' (r|b) ' | sed -e 's/.* //' | sort | uniq -c | sort -n | c++filt | egrep -v '( \.LC|::__FUNCTION__|::_site)' ...lots of symbols only occuring once... 2 CSWTCH.1781 2 FTM_GEO_SKY 2 PANEL_PICKS 2 sTesterName 2 PREVIEW_HPAD 2 PANEL_PROFILE 2 FTM_REBUILD_VBO 2 LSCRIPTDataSize 2 MANIPULATOR_IDS 2 FTM_CULL_REBOUND 2 LSCRIPTTypeNames 2 COLLAPSED_BY_USER 2 FTM_CREATE_OBJECT 2 FTM_UPDATE_WLPARAM 2 FTM_PROCESS_OBJECTS 2 LSCRIPTStateBitField 2 t_panel_group_general 2 PREVIEW_TEXTURE_HEIGHT 2 WEARABLE_NAME_COMPARATOR 2 icon_m 2 icon_r 2 shader 2 icon_pg 2 NEW_LINE 2 t_places 2 (anonymous namespace)::gResponsePtr 3 t_inventory 3 empty_string 3 gDirOpposite 3 LLVORBIS_CLIP_MAX_SAMPLES 3 LLVORBIS_CLIP_REJECT_SIZE 3 LLVORBIS_CLIP_REJECT_SAMPLES 3 LLVORBIS_CLIP_MAX_SAMPLE_DATA 3 t2 4 r3 5 t1 8 OGL_TO_CFR_ROTATION 8 r2 12 r1 52 r 795 std::__ioinit The fact that std::__ioinit occurs 795 times is because it's used in the llinfos and llwarns etc macros as well, in many different places. The t2, r3, t1, r2, r1 and r, are static variables (in .cpp files) of that name used in several places in the code. Different symbols really. In other words, nothing wrong here. But, this is WITH this patch. ******START HERE****** Without the patch and leaving out the symbols from above, the output becomes: 8 VISIBILITY_HIDDEN 8 VISIBILITY_DEFAULT 8 VISIBILITY_VISIBLE 8 VISIBILITY_INVISIBLE 24 TEXTBOX_MAGIC_TOKEN 49 AIR_SCA_AVG 49 AIR_SCA_INTENS 49 gAirScaSeaLevel 351 DEFAULT_METRIC_NAME 518 NEG_X_AXIS 518 NEG_Y_AXIS 518 NEG_Z_AXIS 518 X_AXIS 518 Y_AXIS 518 Z_AXIS Thus, these are the duplicates (and how often) that are still in the code. Agreed, not many - but that is because you already applied SNOW-800 in the past. This should address your, | I fail to see how any of those changes "massively prevents object duplication". Next let me comment on, | I don't disagree with any of them but I don't see much value in any either. I completely agree with you, I don't see much value either. Not in terms of FPS improvements, not in link or compile times, and not even in making the code more robust. However, since I had to be SURE - and therefore had to redo SNOW-800... I ended up with this patch, and even though a "code clean up" and with the bonus of getting rid of several thousand duplicated constants here and there... I saw no reason not to commit it. | Sure, there are different ways to skin a cat and some are better. | Still, these changes will likely make upcoming merges conflict | annoyingly and make the life of the maintainers (i.e. mine and Oz) | horrid. Okaayyy.. now here is where we disagree ;). I think you're extremely exaggerating with the 'horrid', more over, I'm willing to make a bet that you will get no collision at all (ok, not really, because you have internal-knowledge of what other people are working on ;). The patch can be divided in three parts: 1) A single hunk of 139 lines in indra/llcharacter/llanimationstates.cpp that is merely a "style" fix (almost: it's 'bad coding' in that causes 139 times a call to a malloc, free and a copy constructor of LLUUID while utterly unnecessary. I ONLY added this to THIS patch, because this change was part of the *original* SNOW-800 patch (which is in snowglobe 1.x, where these lines were MOVED from a header to the .cpp file), but for some reason this change wasn't copied to viewer 2. I have no objections whatsoever if you decide not to apply this hunk, but seriously - how often is THAT part of the code changed by other people? It's a list of UUID's for crying out loud. It will NOT be changed for MONTHS to come. It will really not cause any collisions, or merge horror, because nobody else changes that code, ever. 2) Four lines in indra/llcommon/llavatarconstants.h that gets rid of the VISIBILITY_* duplications. Surely those four lines won't give you a merge problem either. It's extremely unlikely that, until everyone merged this, anyone will change code near those lines, and even if some person does - the fix of such collision would be extremely trivial. Likewise one line in indra/llcommon/lllslconstants.h for TEXTBOX_MAGIC_TOKEN. One line in indra/llcommon/llmetricperformancetester.h for DEFAULT_METRIC_NAME etc... minimal changes that in practise are very unlikely to even cause a collision before this is merged to everyone, and are faster to fix if it happens than it took you to read this post. 3) A serious code cleanup (and getting rid of the duplicates) regarding the AIR_SCA (AirSca) calculations. I say serious, because the old code is hardly "maintainable" and very unclear. If you have a hard time reviewing it then I'm sure that is because it nearly impossible to follow what the OLD code did... Again, yes, the old code worked... so why apply the patch when there is a risk for collisions? Well, in this case it's my opinion that the old code is SO bad-- that now the work of cleaning it up is done-- it's worth the risk... especially because in the end, it only changes things very locally (indra/newview/llvosky.cpp), again not likely at all that anyone else will be making changes to that code right now (especially since they'd need to understand the code first - heheh, highly unlikely!). Let me know what your decision is, so I can adapt my final test repository of the collection of the VWR* jiras that I currently have pending on the review board. Thanks for reading (sorry it was so long), Aleric - Aleric ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/81/#review221 ----------------------------------------------------------- On Jan. 16, 2011, 5:53 a.m., Aleric Inglewood wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/81/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2011, 5:53 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > Turns out that most of my SNOW-800 patch was included in Viewer 2 (albeit > without crediting me). > However, not everything was used and some more cleaning up was possible. > > After this patch, and when compiling with optimization, there are no > duplicates left > anymore that shouldn't be there in the first place: apart from the debug > stream > iostream guard variable, there are several static variables with the same > name (r, r1, > r2, etc) but that indeed actually different symbol objects. Then there are a > few > constant POD arrays that are duplicated a hand full of times because they are > accessed with a variable index (so optimizing them away is not possible). I > left them > like that (although defining those as extern as well would have been more > consistent > and not slower; in fact it would be faster theoretically because those arrays > could > share the same cache page then). > > > This addresses bug VWR-24312. > http://jira.secondlife.com/browse/VWR-24312 > > > Diffs > ----- > > doc/contributions.txt 422f636c3343 > indra/llcharacter/llanimationstates.cpp 422f636c3343 > indra/llcommon/llavatarconstants.h 422f636c3343 > indra/llcommon/lllslconstants.h 422f636c3343 > indra/llcommon/llmetricperformancetester.h 422f636c3343 > indra/llmath/llcamera.h 422f636c3343 > indra/llmath/llcamera.cpp 422f636c3343 > indra/newview/llviewerobject.cpp 422f636c3343 > indra/newview/llvoavatar.cpp 422f636c3343 > indra/newview/llvosky.h 422f636c3343 > indra/newview/llvosky.cpp 422f636c3343 > > Diff: http://codereview.secondlife.com/r/81/diff > > > Testing > ------- > > Compiled with optimization and then running readelf on the executable to find > duplicated symbols. > > > 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
