[opensource-dev] Review Request: Squared all dist_vec() based comparisons and other dist_vec() operations where sensible.

Ricky kf6kjg at gmail.com
Fri Mar 11 08:43:34 PST 2011


Thanks for the review! Yes, I am familiar with those "principles of
optimization" - yet it seemed to just feel wrong to leave it alone... :P

As to the variables that are initialized with high numbers, there has to be
a better way: if these were standard for-loops, I would just initialize
the first value with the first item in the list, and then start the loop at
the second value, if any.  However, these are a iterator style I am not yet
familiar enough with to bend that far.  Any suggestions?

Ricky
Cron Stardust

On Fri, Mar 11, 2011 at 3:53 AM, Boroondas Gupte <sllists at boroon.dasgupta.ch
> wrote:

>    This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
>
> 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<http://codereview.secondlife.com/r/199/diff/1/?file=1187#file1187line337> (Diff
> revision 1)
>
> void LLNetMap::draw()
>
>   337
>
> 		F32 closest_dist = F32_MAX;
>
> 337
>
> 		F32 closest_dist_squared = F32_MAX;
>
>   Maybe add a short comment here, that this value is meant to be overwritten in the loop below it.
>
>
>    indra/newview/llpanelpeople.cpp<http://codereview.secondlife.com/r/199/diff/1/?file=1188#file1188line161> (Diff
> revision 1)
>
> public:
>
>   161
>
> 		F32 dist1 = dist_vec(item1_pos, me_pos);
>
>  161
>
> 		F32 dist1 = dist_vec_squared(item1_pos, me_pos);
>
>   162
>
> 		F32 dist2 = dist_vec(item2_pos, me_pos);
>
>  162
>
> 		F32 dist2 = dist_vec_squared(item2_pos, me_pos);
>
>   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.)
>
>
>    indra/newview/llselectmgr.cpp<http://codereview.secondlife.com/r/199/diff/1/?file=1189#file1189line6573> (Diff
> revision 1)
>
> bool LLSelectMgr::selectionMove(const LLVector3& displ,
>
>   6573
>
> 		F32 min_dist = 1e+30f;
>
> 6573
>
> 		F32 min_dist = 1e+30f;
>
>   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<http://codereview.secondlife.com/r/199/diff/1/?file=1189#file1189line6574> (Diff
> revision 1)
>
> bool LLSelectMgr::selectionMove(const LLVector3& displ,
>
>   6574
>
> 		LLVector3 obj_pos;
>
> 6574
>
> 		LLVector3 obj_pos;
>
>   6575
>
> 		for (LLObjectSelection::root_iterator it = getSelection()->root_begin();
>
>  6575
>
> 		for (LLObjectSelection::root_iterator it = getSelection()->root_begin();
>
>   6576
>
> 			 it != getSelection()->root_end(); ++it)
>
>  6576
>
> 			 it != getSelection()->root_end(); ++it)
>
>   6577
>
> 		{
>
> 6577
>
> 		{
>
>  6578
>
> 			obj_pos = (*it)->getObject()->getPositionEdit();
>
>  6578
>
> 			obj_pos = (*it)->getObject()->getPositionEdit();
>
>   6579
>
> 			 6579
>
> 			   6580
>
> 			F32 obj_dist = dist_vec(obj_pos, LLViewerCamera::getInstance()->getOrigin());
>
>  6580
>
> 			F32 obj_dist_squared = dist_vec_squared(obj_pos, LLViewerCamera::getInstance()->getOrigin());
>
>   6581
>
> 			if (obj_dist < min_dist)
>
> 6581
>
> 			if (obj_dist_squared < min_dist)
>
>    6582
>
> 			{
>
> 6582
>
> 			{
>
>   6583
>
> 				min_dist = obj_dist;
>
> 6583
>
> 				min_dist = obj_dist_squared;
>
>   6584
>
> 			}
>
> 6584
>
> 			}
>
>  6585
>
> 		}
>
> 6585
>
> 		}
>
>    6586
>
> 		// since the above uses squared values, take the square root.
>
>   6587
>
> 		min_dist = sqrt(min_dist);
>
>   Memory reuse is good, I guess, but having variable names that only describe the variable's content correctly part of the time bothers me.
>
>
>    indra/newview/llselectmgr.cpp<http://codereview.secondlife.com/r/199/diff/1/?file=1189#file1189line6589> (Diff
> revision 1)
>
> bool LLSelectMgr::selectionMove(const LLVector3& displ,
>
>   6587
>
> 		// factor the distance inside the displacement vector. This will get us
>
> 6589
>
> 		// factor the distance inside the displacement vector. This will get us
>
>  6588
>
> 		// equally visible movements for both close and far away selections.
>
> 6590
>
> 		// equally visible movements for both close and far away selections.
>
>  6589
>
> 		min_dist = sqrt(min_dist) / 2;
>
>  6591
>
> 		min_dist = sqrt(min_dist) / 2;
>
>   6590
>
> 		displ_global.setVec(displ.mV[0]*min_dist,
>
>  6592
>
> 		displ_global.setVec(displ.mV[0]*min_dist,
>
>    6591
>
> 							displ.mV[1]*min_dist,
>
>  6593
>
> 							displ.mV[1]*min_dist,
>
>   6592
>
> 							displ.mV[2]*min_dist);
>
>  6594
>
> 							displ.mV[2]*min_dist);
>
>   Off course, this variable already changed meaning during execution before your change, so ... meh.
>
>
> - Boroondas
>
> On March 11th, 2011, 12:06 a.m., Cron Stardust wrote:
>   Review request for Viewer.
> By Cron Stardust.
>
> *Updated March 11, 2011, 12:06 a.m.*
> Description
>
> I looking at the code, trying to find out where/how to add a new feature, when I tripped across one of these and it lit my mental warning bells off.  Vector distance comparisons should, IMHO, always be done squared.  So I did some greppin, manual analysis, and some careful modification, and here's the result.
>
>   Testing
>
> Compiled a test viewer and used it, undertaking some of my normal activities.  Results felt good, but are currently anecdotal.  Any suggestions on how to properly measure this (or even some actual measurement from those already instrumented to measure these things,) would be great!
>
>   *Bugs: * https://jira.secondlife.com/browse/VWR-25126<http://jira.secondlife.com/browse/https://jira.secondlife.com/browse/VWR-25126>
> Diffs
>
>    - doc/contributions.txt (344d4c6d7d7e)
>    - indra/llcharacter/llbvhloader.cpp (344d4c6d7d7e)
>    - indra/llmath/tests/llbbox_test.cpp (344d4c6d7d7e)
>    - indra/newview/llagent.cpp (344d4c6d7d7e)
>    - indra/newview/llfloaterchat.cpp (344d4c6d7d7e)
>    - indra/newview/llhudeffectlookat.cpp (344d4c6d7d7e)
>    - indra/newview/llhudeffectpointat.cpp (344d4c6d7d7e)
>    - indra/newview/llmaniprotate.cpp (344d4c6d7d7e)
>    - indra/newview/llmanipscale.cpp (344d4c6d7d7e)
>    - indra/newview/llnetmap.cpp (344d4c6d7d7e)
>    - indra/newview/llpanelpeople.cpp (344d4c6d7d7e)
>    - indra/newview/llselectmgr.cpp (344d4c6d7d7e)
>    - indra/newview/llspeakers.cpp (344d4c6d7d7e)
>    - indra/newview/llviewerchat.cpp (344d4c6d7d7e)
>    - indra/newview/llvoicevivox.cpp (344d4c6d7d7e)
>    - indra/newview/llworld.cpp (344d4c6d7d7e)
>
> View Diff <http://codereview.secondlife.com/r/199/diff/>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110311/4b2c63b6/attachment-0001.htm 


More information about the opensource-dev mailing list