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 <slli...@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/> >
_______________________________________________ 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
