833414 - remove garbage collection calls to speedup websockets tests by 50%+

Status: RESOLVED WONTFIX

(Core :: Networking: WebSockets, defect)

Description

[Jason Duell]

Attachment: v1: remove garbage collection calls from websockets tests

+++ This bug was initially created as a clone of Bug #830920 +++ 

So we got test_websocket_simple.html down from 300sec per run to 30 in bug
830920.  Removing this (pointless?) test entirely (we have lots of other ping tests for websockets: I'm not sure what testing value all the forcegc() calls are adding) drops it down to 10 seconds.  Removing GC calls in test_websocket.html drops that test from 25sec -> 10sec on my box.

I don't know if we really need this GC test coverage (and so much of it), as I know little about GC or how we need to test it.

billm: rumor has it on IRC that may have added (or know who did) recent changes that make GC much slower in debug mode.  True?  Is there a chance we can speed that up a bit?  Otherwise I think we need an audit of our test usage generally so we don't bog down our test bots.

holding off on review here until I hear from billm in case this should be morphed into a "speed up debug GC" bug.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This seems like a good change to me. The more forceGC calls we can remove, the better. They definitely don't improve testing at all--they make it worse, since actual code won't have those calls. If we depend on these calls, it's a sign of brokenness somewhere.

Debug-mode GCs have gotten slower by ~2x or so I would guess. However, this extra checking is definitely valuable for us. I don't think there's anything obvious we could do to speed it up, and I'd rather not turn it off.

Comment 2

[Edmund Wong (:ewong)]

Comment on attachment 704945 [details] [diff] [review]
v1: remove garbage collection calls from websockets tests

drive-by nit:

diff --git a/content/base/test/test_websocket_basic.html b/content/base/test/test_websocket_basic.html
 function testWebSocket3() {
-  gTestElement.textContent = "Running testWebSocket3() [can take a little while]";
-
-  const displayInterval = 10;
-  const testCount = 10;
-  const testMessage = "test message 3.";
-
-  var messageCount = 0;
-
-  ws = new WebSocket(kUrl, "test");
-  // Set this before onopen() is called,
-  // otherwise its display would be delayed by forcegc() calls...
-  gTestElement.textContent += "\nSending :";
-  ws.onopen = function(e) {
-    for (var i = 1; i <= testCount; ++i) {
-      forcegc();
-      if (i % displayInterval == 1) {
-        // Actual display is delayed by forcegc() calls...
-        gTestElement.textContent += " " + i;
-      }
-      ws.send(testMessage + i);
-    }
-    forcegc();
-    gTestElement.textContent += " end";
-    ws.send("end");
-
-    // Set this before onmessage() is called, so it is displayed once only.
-    gTestElement.textContent += "\nReceived:";
-  };
-  ws.onclose = function(e) {
-    is(messageCount, testCount + 1, "[3] Number of received messages");
-    ok(e.wasClean, "[3] Connection closed cleanly");
-
-    SimpleTest.executeSoon(testWebSocket4);
-  };
-  ws.onerror = function(e) {
-    ok(false, "[3] onerror() should not have been called!");
-    gTestElement.textContent += "\nonerror() should not have been called!";
-    SimpleTest.executeSoon(SimpleTest.finish);
-  };
-  ws.onmessage = function(e) {
-    forcegc();
-    ++messageCount;
-    if (messageCount > testCount)
-      is(e.data, "end", "[3] Received message");
-    else
-      is(e.data, testMessage + messageCount, "[3] Received message");
-    if (messageCount % displayInterval == 1) {
-      // Actual display is delayed by forcegc() call(s)...
-      gTestElement.textContent += " " + messageCount;
-    }
-  };
-}
-
-/**
- * Sends a huge test message, then receives it, then closes the WebSocket from client-side.
- */
-function testWebSocket4() {
   gTestElement.textContent = "Running testWebSocket4()";

Shouldn't this be "Running testWebSocket3()"?

Comment 3

[Jason Duell]

Attachment: v2

Thanks for noticing that.  I've also renamed test5 to test4, so we don't have a "missing test".  Ah, the perils of hard-coding test counts... :)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 705278 [details] [diff] [review]
v2

We should not make some of the tests useless.
The forcegc tests are there because of Bug 572975, which is a behavior
we don't want to regress and we must have tests for it.

Comment 5

[Jason Duell]

Comment on attachment 705278 [details] [diff] [review]
v2

ah, right, OK.   Well, can we get rid of at least some of the forcegc() calls here?  We're consuming about a minute per platform per mochitest run to get coverage for this right now.  Perhaps that's ok, but seems like a lot of bot resources to me.  Perhaps we can scrap the test_websocket_basic test at least, or drop it's loop count from 10 to 2 or something?

Comment 6

[Patrick McManus [:mcmanus]]

fwiw I've never known the purpose of those forcegc() calls. They predate my involvement with those tests.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

That is why we have blame/annotate which clearly points to Bug 572975 and shows that there is a good
reason for most of those forcegc calls.