Good idea.

Off course, the two main rules of optimization are:
1. Don't optimize.
2. Don't optimize yet (experts only).

Though, comparing squared distances instead of actual distances can probably be considered standard practice and both the proposed change as well as the resulting code are still easy to understand. Also, you already measured the operations in isolation and it's unlikely we'll have hardware that calculates square roots faster than it multiplies in the foreseeable future, so this is different than, say, bit-shifting with addition to replace integer multiplication. Still, it'd be nice to have some measurements of how this affects in-viewer performance.

Some comments about the actual code:

indra/newview/llnetmap.cpp (Diff revision 1)

Maybe add a short comment here, that this value is meant to be overwritten in the loop below it.

1. Cron Stardust March 12, 2011, 6:55 p.m. (March 12, 2011, 6:55 p.m.)

   Good idea!  I'll do so.

indra/newview/llpanelpeople.cpp (Diff revision 1)

For clarity, please rename these variables to dist1_squared and dist2_squared, too. Or eliminate them by calling dist_vec_squared right in the return statement:
		return dist_vec_squared(item1_pos, me_pos) < dist_vec_squared(item2_pos, me_pos);

(A bit long, but still shorter than the lines right above it.)

1. Cron Stardust March 12, 2011, 6:57 p.m. (March 12, 2011, 6:57 p.m.)

   I think I'll go with the idea of remove the extraneous variables.  I don't think it makes the code that much less readable or understandable, and it leaves less for the compiler to have to optimize away. (Not that that last is a reason that can stand up on its own; I simply use it as spice for a stronger reason...)

indra/newview/llselectmgr.cpp (Diff revision 1)

This should probably start at 1e60f, now. Though ... if there is no specific reason for the previous 1e30f value, just make it F32_MAX, I guess. Might also be worth a comment, that it will be overwritten in the loop below.

indra/newview/llselectmgr.cpp (Diff revision 1)

Memory reuse is good, I guess, but having variable names that only describe the variable's content correctly part of the time bothers me.

1. Oz Linden March 12, 2011, 6:46 a.m. (March 12, 2011, 6:46 a.m.)

   Agree with Boroondas here - the variable used inside the loop should be min_dist_squared, not min_dist.
   
   Leave the memory optimization to the compiler.

2. Cron Stardust March 12, 2011, 6:58 p.m. (March 12, 2011, 6:58 p.m.)

   No problem.  I was just using what I had on hand. :)

indra/newview/llselectmgr.cpp (Diff revision 1)

Off course, this variable already changed meaning during execution before your change, so ... meh.

1. Oz Linden March 12, 2011, 6:47 a.m. (March 12, 2011, 6:47 a.m.)

   but since you're touching it anyway, fix it

2. Cron Stardust March 12, 2011, 6:59 p.m. (March 12, 2011, 6:59 p.m.)

   QSL: I'm on it! :D

If the comments here are addressed, I think this is probably a good idea.  I agree that making changes that might make code less clear is usually not justified by claims of performance benefit, but done carefully this change can almost certainly be beneficial and not cost any real clarity.

indra/llcharacter/llbvhloader.cpp (Diff revision 1)

I think it would be clearer to either add a new constant POSITION_MOTION_THRESHOLD_SQUARED or to write these like

 ... < (POSTION_MOTION_THRESHOLD*POSITION_MOTION_THRESHOLD).

(and same convention elsewhere)

1. Boroondas Gupte March 12, 2011, 2:13 p.m. (March 12, 2011, 2:13 p.m.)

   For this specific line, you probably mean ">", not "<", do you?
   
   Also, what's the convention are you proposing? Grouping the multiplied values together by braces? Or leaving away the spaces around the multiplication operator?

2. Oz Linden March 12, 2011, 4:09 p.m. (March 12, 2011, 4:09 p.m.)

   I meant putting the constant being squared inside parens with no spaces.  Really, if it's a constant I think it's probably better to make another constant with _SQUARED appended.
   
   While I'm being nit-picky - I think it's clearer when wrapping an expression across lines to put the operator at the beginning of the second line instead of the end of the first line:
   
   (dist_vec_squared(LLVector3(ki_prev->mPos), first_frame_pos) 
    > (POSITION_MOTION_THRESHOLD*POSITION_MOTION_THRESHOLD))
   

3. Cron Stardust March 12, 2011, 6:54 p.m. (March 12, 2011, 6:54 p.m.)

   Oz, I was thinking along those lines (creating or changing the constant,) but I wasn't sure if it would be within the scope of this.  After further thought, and evaluating the input thus far, I've come to the conclusion that it is within the scope.  A new diff incorporating the notes thus far mentioned will be forthcoming.

4. Cron Stardust March 17, 2011, 10:44 a.m. (March 17, 2011, 10:44 a.m.)

   Actually, it's not wrapped - that's a figment of your screen width. :)  However, this is also now the old edition of the patch, the new revision changes POSITION_MOTION_THRESHOLD into POSITION_MOTION_THRESHOLD_SQUARED.

Thanks for the changes. Some more comments from me:

indra/newview/llselectmgr.cpp (Diff revisions 1 - 2)

While were at it, the original author probably meant "factor into" when they wrote "factor inside". And although the comment doesn't begin with a *complete* sentence, I'd start it with a capital letter when further sentences follow.
So make this
		// Factor the distance into the displacement vector. This will get us
		// equally visible movements for both close and far away selections.

1. Cron Stardust March 13, 2011, 9:35 a.m. (March 13, 2011, 9:35 a.m.)

   Good idea, but none of the other comments in the file start with a capital.  Also, I'm not sure what the real meaning of the below is...

indra/newview/llselectmgr.cpp (Diff revisions 1 - 2)

Notwithstanding the comment above this code, I don't think the notion of a "factored minimal distance" makes any sense at all. (I might be mistaken, as I'm not a native speaker of English.)

If we can't think of a better name (e.g. based on the actual geometric meaning of this intermediary result), just describe what we're storing here, e.g. with an ugly name like half_sqrt_of_min_dist.

Because the variable is only used very locally and the computation of its value isn't too complicated, just naming it "tmp" or similar might also be acceptable, and still better than a name purporting a wrong or confusing meaning.

1. Boroondas Gupte March 13, 2011, 5:35 a.m. (March 13, 2011, 5:35 a.m.)

   Oh, and remove the trailing whitespace.

2. Cron Stardust March 13, 2011, 9:42 a.m. (March 13, 2011, 9:42 a.m.)

   Good call: I too have no idea what the meaning of "sqrt(min_dist) / 2" truely is, and I am a native speaker of English!  I assumed that the sqrt was what was doing the factoring above, but I could be wrong; factoring to me means separating a number into its constituent factors, ie: 16 = 4 * 2 * 2.  (Note that I'm not taking it into its prime factors, just the more general "factoring.") If my assumption is correct, then my variable name makes sense as it is the "min_dist" factored, otherwise I need some help grasping what was intended my this math.
   
   And thanks for the point-out on the trailing whitespace! I hadn't seen them, even though my editor has the whitespace set visible for just such a purpose!

3. Boroondas Gupte March 13, 2011, 12:46 p.m. (March 13, 2011, 12:46 p.m.)

   I'm pretty sure no "factoring" in the sense of "split into factors" takes place here. As written above, I think that when writing "factor [A] inside [B]", the comment author actually meant "factor [A] into [B]" (in the meaning of "work [A] into [B]", i.e., take [A] into account, make it part of the result [B], have [A] influence [B]).
   
   The function at hand gets called when objects are being moved with a SpaceNavigator or joystick. I'm not completely sure, but I guess the math here is to take perspective foreshortening into account, so that you get the same apparent (i.e. onscreen) movement for far and nearby selections for the same joystick input. (I.e., in 3D space, the far selections will actually move faster than nearby ones would.) I'm not really sure why the nearestby object of the selection determines the selection's distance, rather than -say- the center of mass.
   
   Though, isn't perspective foreshortening inverse linear to the distance, due to the http://en.wikipedia.org/wiki/Intercept_theorem ? Did someone confuse lines and areas here? Or would actual scaling with the distance be too extreme, while no scaling would feel strange too, so the square root is some kind of compromise?
   
   The more I think about it, the more obscure this calculation seems to me.

4. Oz Linden March 17, 2011, 10:06 a.m. (March 17, 2011, 10:06 a.m.)

   Let's not try to fix the naming here with respect to whatever 'factor' was supposed to mean.
   

indra/llmath/tests/llbbox_test.cpp (Diff revision 2)

While we're changing this anyway, braces around the assigned define value probably won't hurt:
#define APPROX_EQUAL(a, b)   (dist_vec_squared((a),(b)) < 1e-10)

They protect the define from unexpected operator precedence issues when used in certain contexts, so that it acts more similar to a function.

Though, one has to wonder anyway, why a define is used here rather than a function. (Which could be declared inline, if really, really necessary.)

1. Cron Stardust March 13, 2011, 10:04 a.m. (March 13, 2011, 10:04 a.m.)

   The parens makes perfect sense: the lesser-than operator is pretty low on the precedence totem pole.  I'll add them to the next revision, after some further discussion on some of the other topics.
   
   As to why it's not a function: I have no idea.  C++ has way too many ways of doing the same task! :P
   
   

2. Boroondas Gupte March 13, 2011, 12:34 p.m. (March 13, 2011, 12:34 p.m.)

   It's almost like perl, isn't it? ;-)

3. Oz Linden March 17, 2011, 10:05 a.m. (March 17, 2011, 10:05 a.m.)

   It's not as good as perl :-)

indra/newview/llfloaterchat.cpp (Diff revision 2)

If storing return values of functions (or inline computations), just so we don't have to call the function (or perform the calculation) twice to get its square is a reoccurring pattern, we might want a helper function that'll save us from having to introduce extra variables just for this purpose:

// Either
F32 sqr(F32 base) { return (base * base); } // Isn't something like this already available in the standard library or llmath? Maybe even as a template?

// and then here
			F32 distance_squared = dist_vec_squared(pos_agent, chat.mPosAgent);
			if (distance_squared > sqr(gAgent.getNearChatRadius()))
			// ...

// or, if this mainly occurs comparisons between distances to other values
bool dist_vec_leq( LLVector3 first_position, LLVector3 second_position, F32 distance)
{
	return ( dist_vec_squared(first_position, second_position) <= (distance * distance) );
}

// and then here

			if (!dist_vec_leq(pos_agent, chat.mPosAgent, gAgent.getNearChatRadius()))
			// ...

Off course, where intermediary variables help readability or understandability, we should prefer them, but I don't think that's the case here.

1. Boroondas Gupte March 13, 2011, 5:39 a.m. (March 13, 2011, 5:39 a.m.)

   [...] if this mainly occurs *at* comparisons [...]

2. Cron Stardust March 13, 2011, 10:08 a.m. (March 13, 2011, 10:08 a.m.)

   Good thoughts, however I think I'll have to defer to Oz on this one: I don't feel qualified to say one way or the other in this case.  To me a helper function would clean up several lines of code in the codebase, but is it called for, and is it in the scope of this change?  I cannot say...  Oz?

3. Boroondas Gupte March 13, 2011, 12:44 p.m. (March 13, 2011, 12:44 p.m.)

   Should we tag the jira issue with the label "needs-design" or is that for user-visible design only?

4. Oz Linden March 17, 2011, 10:04 a.m. (March 17, 2011, 10:04 a.m.)

   needs-design is just for UI
   
   While conceptually I like the idea of the helper function, the main motivation for this is performance, so I'd say leave it as is without the helper function.