Bug 20949 - Catch repeated messages in Inspector Controller to limit memory usage
Summary: Catch repeated messages in Inspector Controller to limit memory usage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-19 15:05 PDT by Kevin McCullough
Modified: 2008-09-24 01:39 PDT (History)
1 user (show)

See Also:


Attachments
patch (9.71 KB, patch)
2008-09-19 17:00 PDT, Kevin McCullough
oliver: review-
Details | Formatted Diff | Diff
new patch with test (12.97 KB, patch)
2008-09-22 13:58 PDT, Kevin McCullough
timothy: review-
Details | Formatted Diff | Diff
new patch with changes (13.56 KB, patch)
2008-09-22 15:05 PDT, Kevin McCullough
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin McCullough 2008-09-19 15:05:27 PDT
The Web Inspector captures repeated messages and collapses them, but this could be done in the Inspector Controller, and would reduce the memory usage even more since then we would not be keeping each message in native code.
Comment 1 Kevin McCullough 2008-09-19 17:00:51 PDT
Created attachment 23586 [details]
patch
Comment 2 Oliver Hunt 2008-09-19 17:06:50 PDT
Comment on attachment 23586 [details]
patch

static ConsoleMessage* previousMessage = 0;

Is unsafe as it is global whereas InspectorController is per WebView (afaiaa) so you could have to webviews spewing the same error and you would get excitingly confused.

I'm also not sure what happens if i had a case where i had a repeating group of messages -- say trying to load malformed content or some such could lead to repeating groups of messages, but because it's not the same message over and over in immediate succession it would not be caught.
Comment 3 Kevin McCullough 2008-09-22 09:20:06 PDT
Yes good catch on the static, I'll change that.

wrt the repeating groups we currently have no plans to try and detect that.  To do that you need to answer questions like, how big is a group? and how do you represent their repetition?  I think for the vast majority of the cases, this is the right solution.
Comment 4 Kevin McCullough 2008-09-22 13:58:18 PDT
Created attachment 23668 [details]
new patch with test

Fixes Ollie's comments and some bugs I had when adding repeated messages dynamically (as opposed to just the ones in the InspectorController
Comment 5 Oliver Hunt 2008-09-22 14:08:58 PDT
Comment on attachment 23668 [details]
new patch with test

r=me
Comment 6 Timothy Hatcher 2008-09-22 14:27:31 PDT
Make sure you add m_previousMessage = 0; in clearConsoleMessages.
Comment 7 Timothy Hatcher 2008-09-22 14:31:35 PDT
Please fix the _resourceMessage_resourceMessageRepeatCountElement property name.

It should be: _resourceMessageRepeatCountElement

Also it seems odd that msg.repeatCount is sometimes the difference, if I understand correctly.
Comment 8 Timothy Hatcher 2008-09-22 14:37:08 PDT
Comment on attachment 23668 [details]
new patch with test

+            var previousMessageElement = this.currentGroup.messagesElement;
+            previousMessageElement.removeChild(previousMessageElement.lastChild);
+            previousMessageElement.appendChild(msg.toMessageElement());

previousMessageElement is not the right name for that variable, messagesElement is a better name.

+            msg.repeatCount -= this.previousMessage.repeatCount;

This line deserves a comment.

Even better would be to assign the difference of the repeat count to a new property on the msg, so clients know they are getting the difference. Better yet, just store it in a loval variable, and pass that number to addMessageToResource. In addMessageToResource if the number is > 0 then do the repeat path.

-            msg.resource = WebInspector.resourceURLMap[msg.url];
+            msg.resource = WebInspector.resourceURLMap[msg.url]; 

Don't land the extra space at the end of that line.

r- until some of these are fixed.
Comment 9 Kevin McCullough 2008-09-22 15:05:08 PDT
Created attachment 23673 [details]
new patch with changes
Comment 10 Kevin McCullough 2008-09-23 11:38:39 PDT
Committed revision 36809.
Comment 11 Kevin McCullough 2008-09-23 11:39:09 PDT
my bad.  Wrong bug
Comment 12 Kevin McCullough 2008-09-23 17:06:37 PDT
Committed revision 36817.