Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)

2011-02-04 Thread Merov Linden

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/#review331
---

Ship it!


OK, did a merge and complete TC build on all platforms: no merge issue, no 
build error. Clear to go. 

- Merov


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

Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)

2011-02-03 Thread Aleric Inglewood


 On Jan. 31, 2011, 6:44 p.m., Merov Linden wrote:
  @Aleric: OK, you convinced me on all accounts *except* for the ease of 
  merge which I want to test myself before giving my blessing to this patch. 
  You wouldn't be the first one be to claim that it merges and create some 
  issues unwittingly. If you could post the URL of an hg repo with your 
  patch, I'll be happy to give it a spin building on all platforms.
  
  BTW, thanks for the super detailed comment: very interesting stuff.

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]


- Aleric


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/#review300
---


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

Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)

2011-01-31 Thread Merov Linden

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/#review300
---


@Aleric: OK, you convinced me on all accounts *except* for the ease of merge 
which I want to test myself before giving my blessing to this patch. You 
wouldn't be the first one be to claim that it merges and create some issues 
unwittingly. If you could post the URL of an hg repo with your patch, I'll be 
happy to give it a spin building on all platforms.

BTW, thanks for the super detailed comment: very interesting stuff.

- Merov


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

Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)

2011-01-21 Thread Aleric Inglewood


 On Jan. 14, 2011, 1:31 p.m., Boroondas Gupte wrote:
  indra/llcommon/lllslconstants.h, line 184
  http://codereview.secondlife.com/r/81/diff/1/?file=392#file392line184
 
  Yay for having type modifiers after the core type name. Makes them much 
  easier to understand, especially when there are several cascaded ones. :-)
 
 Aleric Inglewood wrote:
 Yeah, I'm strongly convinced that TYPE const is superior in anyway over 
 const TYPE.
 See http://www.xs4all.nl/~carlo17/cpp/const.qualifier.html for the 
 reasoning.
 In one line: all type qualifiers work to the left, it's best to PUT them 
 on the
 left so the whole type is logically uniform in it's construction. The 
 fact that
 you can legally type 'const TYPE' is just a historically grown misfortune.
  


Of course I meant.. All type qualifiers work to the left, so it's best to PUT 
them on the _right_ ..., as in:
TYPE QUALIFIER : The Qualifier changes the TYPE on it's left, so that what 
first was TYPE now becomes TYPE QUALIFIER.

[Where QUALIFIER is not just const, volatile, restrict, but also * and .
The only exception where any qualifier works to the right is where 'const'
(volatile and restrict, but NOT * an ) works on a base type. Surely it needs
getting used when one changes style, but this one is so logical that already
after a single week you don't understand anymore how you were ever able to
use to previous format style.

While on the topic of coding style and types. The above is a good reason to
put * (and ) that are part of types *against* the TYPE they work on.
That way one can easily recognize the difference between the unary operator,
the binary operator and the type qualifier:

A* b = *c * d + e; // The type qualifier has no space on it's left (and
   // changes the type A), the binary operator (multiplication)
   // has spaces on both sides, while the unary operator
   // (dereference) works to it's right side and has no
   // space on the right.]

[[ Ie,

#include iostream

struct A { int i; };

int main()
{
  A const a[7] = { 0, 1, 2, 3, 4, 5, 6 };
  int const two = 2;

  int const* c = two;
  int const  d = 3;
  A* const e = a[0];

  A* b = *c * d + e;

  std::cout  b-i  std::endl;
}

]]


- Aleric


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/#review153
---


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

Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)

2011-01-21 Thread Aleric Inglewood


 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)'
 r const1
0004 r const2
 b const3
 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)'
 r const1
0004 r const2
 b const3
 W nonPOD::nonPOD(int)

Finally we can look at the end result, the linked executable:

$ nm -C test | egrep '(const[0-9]|POD)'
0040071c r const1
00400724 r const1
00400720 r const2
00400728 r const2
00600b10 b const3
00600b14 b const3
004005d2 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
006009f0 b const3
006009f4 b const3

In other words, optimizing is able to
get rid of POD constants, but not of
non-POD constants: the compiler doesn't

Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)

2011-01-20 Thread Merov Linden

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/#review221
---


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

- Merov


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

Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)

2011-01-16 Thread Aleric Inglewood


 On Jan. 14, 2011, 1:31 p.m., Boroondas Gupte wrote:
  indra/llcommon/lllslconstants.h, line 184
  http://codereview.secondlife.com/r/81/diff/1/?file=392#file392line184
 
  Yay for having type modifiers after the core type name. Makes them much 
  easier to understand, especially when there are several cascaded ones. :-)

Yeah, I'm strongly convinced that TYPE const is superior in anyway over const 
TYPE.
See http://www.xs4all.nl/~carlo17/cpp/const.qualifier.html for the reasoning.
In one line: all type qualifiers work to the left, it's best to PUT them on the
left so the whole type is logically uniform in it's construction. The fact that
you can legally type 'const TYPE' is just a historically grown misfortune.
 


- Aleric


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/#review153
---


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

[opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)

2011-01-14 Thread Aleric Inglewood

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/
---

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 b0bd26c5638a 
  indra/llcharacter/llanimationstates.cpp b0bd26c5638a 
  indra/llcommon/llavatarconstants.h b0bd26c5638a 
  indra/llcommon/lllslconstants.h b0bd26c5638a 
  indra/llcommon/llmetricperformancetester.h b0bd26c5638a 
  indra/llmath/llcamera.h b0bd26c5638a 
  indra/llmath/llcamera.cpp b0bd26c5638a 
  indra/newview/llviewerobject.cpp b0bd26c5638a 
  indra/newview/llvoavatar.cpp b0bd26c5638a 
  indra/newview/llvosky.h b0bd26c5638a 
  indra/newview/llvosky.cpp b0bd26c5638a 

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

Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)

2011-01-14 Thread Boroondas Gupte

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/81/#review153
---



indra/llcharacter/llanimationstates.cpp
http://codereview.secondlife.com/r/81/#comment95

If all these lines are touched anyway, (didn't select all, to avoid an 
unnecessary long review), please either remove the alignment or use spaces 
instead of tabs for aligning, so they will look nice independent of tab display 
length.



indra/llcommon/lllslconstants.h
http://codereview.secondlife.com/r/81/#comment96

Yay for having type modifiers after the core type name. Makes them much 
easier to understand, especially when there are several cascaded ones. :-)


- Boroondas


On Jan. 14, 2011, 12:34 p.m., Aleric Inglewood wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://codereview.secondlife.com/r/81/
 ---
 
 (Updated Jan. 14, 2011, 12:34 p.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 b0bd26c5638a 
   indra/llcharacter/llanimationstates.cpp b0bd26c5638a 
   indra/llcommon/llavatarconstants.h b0bd26c5638a 
   indra/llcommon/lllslconstants.h b0bd26c5638a 
   indra/llcommon/llmetricperformancetester.h b0bd26c5638a 
   indra/llmath/llcamera.h b0bd26c5638a 
   indra/llmath/llcamera.cpp b0bd26c5638a 
   indra/newview/llviewerobject.cpp b0bd26c5638a 
   indra/newview/llvoavatar.cpp b0bd26c5638a 
   indra/newview/llvosky.h b0bd26c5638a 
   indra/newview/llvosky.cpp b0bd26c5638a 
 
 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