91168 - Search sidebar compiles result list even when panel is not visible

Status: VERIFIED FIXED

(SeaMonkey :: Search, defect, P3)

Description

[Jesse Ruderman]

From bug 77460, buglists hang mozilla for long periods of time using all
available CPU:

dbaron is fixing 77460 by making the search sidebar code more efficient.  But
when the search sidebar isn't shown, that code shouldn't be called at all,
because no matter how efficient it is, it's going to slow down the query results
page.  (This includes when the sidebar is collapsed, when the sidebar is turned
off, and when a different panel is visible.)

Comment 1

[Paul Chen]

nav triage team:

Marking p3 and mozilla0.9.5

Comment 2

[Jesse Ruderman]

See also bug 53239, alexa "What's Related" panel contacts server even if panel 
is not visible.

Comment 3

[matt]

The checking code should go in here
http://lxr.mozilla.org/mozilla/source/xpfe/components/search/src/nsInternetSearc
hService.cpp#2515

We don't have a service to detect if the sidebar panel is open though

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Note: comments from bug 54542 indicate that with searches for large numbers of
items (like all NEW bugs in bugzilla) can cause a 15 _minute_ freeze.

Comment 5

[gabriel]

Changing OS to all. I am voting for this because of the bugzilla buglist issue.
Should be topperf IMO.

Comment 6

[Nathan Bidwell]

Did the fix for bug 53239 fix this?

Comment 7

[gabriel]

No.

Comment 8

[Jesse Ruderman]

As long as bug 77460 is open, this bug effectively causes Mozilla to freeze 
whenever I look at the UNCO list.

Comment 9

[Samir Gehani]

Taking.

Comment 10

[Samir Gehani]

Moving to mozilla0.9.7.

Comment 11

[Samir Gehani]

Attachment: Don't compute results is search panel isn't open and "auto open search panel" isn't set.

Comment 12

[Samir Gehani]

jruderman, pchen, or morse, 
please r.

alecf,
please sr.

Comment 13

[Alec Flett]

Comment on attachment 57789 [details] [diff] [review]
Don't compute results is search panel isn't open and "auto open search panel" isn't set.

sr=alecf

Comment 14

[Samir Gehani]

*** Bug 108172 has been marked as a duplicate of this bug. ***

Comment 15

[Stephen P. Morse]

+function isSearchPanelOpen()
+{
+  var isOpen = false;
+
+  if (!sidebar_is_hidden() && !sidebar_is_collapsed())
+  {
+    if (SidebarGetLastSelectedPanel() == "urn:sidebar:panel:search")
+      isOpen = true;
+  }
+
+  return isOpen;

Above simplifies to:

  return (!sidebar_is_hidden() && !sidebar_is_collapsed() &&
           SidebarGetLastSelectedPanel() == "urn:sidebar:panel:search");

Same comment applies to function SidebarGetLastSelectedPanel

Comment 16

[Alec Flett]

thats totally unreadable.

Comment 17

[Alec Flett]

sorry, I didn't offer a better solution though :)
hmm. how about:
function sidebar_is_visible() {
  return !sidebar_is_hidden() && !sidebar_is_collapsed();
}

function sidebar_search_is_last_visited() {
  return (SidebarGetLastSelectedPanel() == "urn:sidebar:panel:search");
}

...
return sidebar_is_visible() && sidebar_search_is_last_visited();

that reads well, anyway :)

Comment 18

[Stephen P. Morse]

Well I don't think it totally unreadable!  Simple "ands" three booleans.

But I do like your solution better.  If samir makes that change, then r=morse.

Comment 19

[Samir Gehani]

OK, so what is the cost of making the function calls vs. the if blocks cause we
are trading one for the other?  

Comment 20

[Samir Gehani]

OK, so I spoke to rogerl about this: he says the cost of two extra function
calls is greater than the cost of the two "if blocks" (even though the current
js inlines "small" functions).  So if the two "if blocks" are considered just as
readable as the function calls then we should use my original patch.  If we care
more about performance than readability in this case then we should use Steve's
patch.  My opinion is that my patch seems like a nice compromise between the
two: it's fairly readable I think.

alecf, morse?

Comment 21

[Alec Flett]

this isn't called from a loop right? I'd say go with the readable solution

Comment 22

[Samir Gehani]

You are right Alec: it isn't called from a loop.  The function to update search
results is essentially one of the page load listeners.

Comment 23

[Jesse Ruderman]

If you put a line break between each literal, you can make the single-if-
statement version more readable.  For example:

  return (   !sidebar_is_hidden() 
          && !sidebar_is_collapsed() 
          && SidebarGetLastSelectedPanel() == "urn:sidebar:panel:search"
         );

By the way, it would be nice if it didn't take three function calls to sidebar 
code just to get the answer to "What panel is visited (if any)?".

Comment 24

[Jesse Ruderman]

Fwiw, I think 

if (a && b && c) 

is easier to read quickly than

if (a && b)
  if (c)

In the second, you have to remember that conjunction and nested ifs are 
equivalent, and mentally convert the two lines into three nested ifs or a 
conjunction of three literals.

Comment 25

[Samir Gehani]

Attachment: Patch revision 2.

Comment 26

[Samir Gehani]

Checked in.

Comment 27

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]