Last Comment Bug 755992 - Reduce about:memory perturbation some more
: Reduce about:memory perturbation some more
Status: RESOLVED FIXED
[MemShrink]
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nils Maier [:nmaier]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-16 20:23 PDT by Nils Maier [:nmaier]
Modified: 2012-05-18 13:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Use RegExp.test(String) (1.13 KB, patch)
2012-05-16 21:58 PDT, Nils Maier [:nmaier]
no flags Details | Diff | Review
Part 2: Avoid Node.createTextNode() in appendElementWithText() (604 bytes, patch)
2012-05-16 21:59 PDT, Nils Maier [:nmaier]
no flags Details | Diff | Review
Part 1: Avoid some appendTextNode() calls (3.81 KB, patch)
2012-05-16 22:01 PDT, Nils Maier [:nmaier]
n.nethercote: review+
Details | Diff | Review
Part 2: Avoid creating extraIndentArray when not necessary (1.21 KB, patch)
2012-05-16 22:04 PDT, Nils Maier [:nmaier]
n.nethercote: review+
Details | Diff | Review

Description Nils Maier [:nmaier] 2012-05-16 20:23:32 PDT
Continue what bug 722972 started.
I got a few patches that will improve the memory consumption of about:memory and hence reduce the perturbation caused by having to generate the page.
Comment 1 Nicholas Nethercote [:njn] 2012-05-16 21:14:06 PDT
We might be butting heads between here and bug 755583... I've already incorporated some of the suggestions you sent me via email, I'll be posting them to bug 755583 later today.
Comment 2 Nils Maier [:nmaier] 2012-05-16 21:23:48 PDT
(In reply to Nicholas Nethercote [:njn] from comment #1)
> We might be butting heads between here and bug 755583... I've already
> incorporated some of the suggestions you sent me via email, I'll be posting
> them to bug 755583 later today.

OK. I thought of bug 755583 as a more "overall" thing, instead of smallish but viable optimization I wanted to propose here. Got five patches... Want me to attach to this bug or kill it and move over to bug 755583?
Comment 3 Nicholas Nethercote [:njn] 2012-05-16 21:49:44 PDT
Eh, might as well do it here, to stop the other bug having too many patches.
Comment 4 Nils Maier [:nmaier] 2012-05-16 21:58:02 PDT
Created attachment 624650 [details] [diff] [review]
Part 1: Use RegExp.test(String)

String.match() create match objects, which in turn will copy the matched part.
RegExp.test() does avoid that, as it returns a primitive boolean.
Comment 5 Nils Maier [:nmaier] 2012-05-16 21:59:51 PDT
Created attachment 624652 [details] [diff] [review]
Part 2: Avoid Node.createTextNode() in appendElementWithText()

Element.createTextNode() does create a new Node object in js-land. Using .textNode directly, that overhead can be avoided.
Comment 6 Nils Maier [:nmaier] 2012-05-16 22:01:59 PDT
Created attachment 624655 [details] [diff] [review]
Part 1: Avoid some appendTextNode() calls

appendTextNode() uses .createTextNode(), which incurs a lot of memory overhead.
In the code appendTextNode() is often used to add some newlines for copy/paste formatting. Some of these newlines can easily be collapsed into existing nodes without creating new ones.
Comment 7 Nicholas Nethercote [:njn] 2012-05-16 22:03:58 PDT
Comment on attachment 624650 [details] [diff] [review]
Part 1: Use RegExp.test(String)

Review of attachment 624650 [details] [diff] [review]:
-----------------------------------------------------------------

Heh, I have the same patch in bug 755583, I already r+'d it because you wrote it.  Let's keep it over there.
Comment 8 Nils Maier [:nmaier] 2012-05-16 22:04:17 PDT
Created attachment 624656 [details] [diff] [review]
Part 2: Avoid creating extraIndentArray when not necessary

Allocating gcthings, such as Objects, always incurs a slight memory penalty. Avoid the extraIndentArray when not needed. This is one of the top spots of object allocations.
Comment 9 Nicholas Nethercote [:njn] 2012-05-16 22:04:19 PDT
Comment on attachment 624652 [details] [diff] [review]
Part 2: Avoid Node.createTextNode() in appendElementWithText()

Review of attachment 624652 [details] [diff] [review]:
-----------------------------------------------------------------

Ditto, except I added a comment :)
Comment 10 Nicholas Nethercote [:njn] 2012-05-16 22:09:55 PDT
Comment on attachment 624655 [details] [diff] [review]
Part 1: Avoid some appendTextNode() calls

Review of attachment 624655 [details] [diff] [review]:
-----------------------------------------------------------------

All of these occur in places that are executed infrequently, so they won't affect performance at all.  But I'll r+ because it makes the code shorter.

Do you want me to land your patches, since I'll be doing the ones in bug 755583?  I'm happy to do so.
Comment 11 Nils Maier [:nmaier] 2012-05-16 22:17:09 PDT
(In reply to Nicholas Nethercote [:njn] from comment #10)
> Do you want me to land your patches, since I'll be doing the ones in bug
> 755583?  I'm happy to do so.

Sure. I don't have any commit access myself, anyway.
Comment 12 Nicholas Nethercote [:njn] 2012-05-16 22:17:17 PDT
Comment on attachment 624656 [details] [diff] [review]
Part 2: Avoid creating extraIndentArray when not necessary

Review of attachment 624656 [details] [diff] [review]:
-----------------------------------------------------------------

About a 3--4% improvement in the cumulative-GC-bytes measurement.
Comment 13 Wayne Mery (:wsmwk, use Needinfo for questions) 2012-05-17 15:08:33 PDT
will this help with the long time it took to me to run about:memory for bug 756261?  
I clicked on unresponsive script at least 5 times.
Comment 15 Nicholas Nethercote [:njn] 2012-05-17 18:22:50 PDT
(In reply to Wayne Mery (:wsmwk) from comment #13)
> will this help with the long time it took to me to run about:memory for bug
> 756261?  
> I clicked on unresponsive script at least 5 times.

Not sure.  But in that case, about:memory's slowness is a consequence of the memory consumption being so high -- it iterates over every object in the JS heap, and if your JS heap is multiple GB that can be very slow.  The real question is why is your JS heap so large, and I discussed that in 756261 comment 4.

Note You need to log in before you can comment on or make changes to this bug.