----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/199/#review444 -----------------------------------------------------------
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/#comment314> 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/#comment315> 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/#comment316> 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/#comment317> 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/#comment318> Off course, this variable already changed meaning during execution before your change, so ... meh. - Boroondas On March 11, 2011, 12:06 a.m., Cron Stardust wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/199/ > ----------------------------------------------------------- > > (Updated March 11, 2011, 12:06 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > 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. > > > This addresses bug 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 > > Diff: http://codereview.secondlife.com/r/199/diff > > > 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! > > > Thanks, > > Cron > >
_______________________________________________ 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
