Closed Bug 755992 Opened 12 years ago Closed 12 years ago

Reduce about:memory perturbation some more

Categories

(Toolkit :: about:memory, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: nmaier, Assigned: nmaier)

Details

(Whiteboard: [MemShrink])

Attachments

(2 files, 2 obsolete files)

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.
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.
(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?
Eh, might as well do it here, to stop the other bug having too many patches.
Attached patch Part 1: Use RegExp.test(String) (obsolete) — Splinter Review
String.match() create match objects, which in turn will copy the matched part.
RegExp.test() does avoid that, as it returns a primitive boolean.
Attachment #624650 - Flags: review?(n.nethercote)
Element.createTextNode() does create a new Node object in js-land. Using .textNode directly, that overhead can be avoided.
Attachment #624652 - Flags: review?(n.nethercote)
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.
Attachment #624655 - Flags: review?(n.nethercote)
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.
Attachment #624650 - Flags: review?(n.nethercote)
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.
Attachment #624656 - Flags: review?(n.nethercote)
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 :)
Attachment #624652 - Flags: review?(n.nethercote)
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.
Attachment #624655 - Flags: review?(n.nethercote) → review+
(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 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.
Attachment #624656 - Flags: review?(n.nethercote) → review+
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.
Attachment #624650 - Attachment is obsolete: true
Attachment #624652 - Attachment is obsolete: true
Attachment #624655 - Attachment description: Part 3: Avoid some appendTextNode() calls → Part 1: Avoid some appendTextNode() calls
Attachment #624656 - Attachment description: Part 4: Avoid creating extraIndentArray when not necessary → Part 2: Avoid creating extraIndentArray when not necessary
(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.
You need to log in before you can comment on or make changes to this bug.