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

   

Thanks. I made a repository available here:
https://bitbucket.org/aleric/viewer-development-storm-955

Due to technical reasons I had to base it on a recent viewer-development commit,
but during the merge I had no collisions except in contributions.txt ;) (while
I made the original patch a few hunderd commits ago).

[PS I added this TWO DAYS ago... but forgot to click on 'Publish'... this kinda sucks a bit]