Remove static (large) buffers from the HTML -> text converter

ASSIGNED
Assigned to

Status

()

P3
minor
ASSIGNED
18 years ago
9 years ago

People

(Reporter: bratell, Assigned: bratell)

Tracking

({memory-footprint})

Trunk
memory-footprint
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

18 years ago
Right now nsHTMLToTXTSinkStream keeps two stacks with a fixed size. Since they 
are fixed size they must be quite big to not hit the limit. This causes the need 
for more memory than necessary and there is still a small risk for failure. 

I did a fix to replace the stacks by dynamic ones and with that reduce initial 
stack size to something closer to what's really needed. 

This is no big thing. It will save a couple of KB of memory for every 
copy/paste, mail send, textfield extraction and whenever else the converter is 
used and that's it.

I will attach the patch here, but it's untested since I cannot run Mozilla since 
a couple of days (registration problem caused by the jar packaging?). I will do 
the tests when that problem has gone away.
(Assignee)

Comment 1

18 years ago
Created attachment 14885 [details] [diff] [review]
Patch to reduce memory usage.

Comment 2

18 years ago
Daniel, are you hitting bug 52879?

Why
+  mTagStack = new nsHTMLTag[mTagStackSize];
but
+  mOLStack = new PRInt32[InitialOLStackSize];
?
Why
+  if(aNewSize == 0)
+    aNewSize = 1;
and not
+  if(aNewSize == 0)
+    return NS_OK;
?

Don't you want to bail out?
+    NS_ASSERTION(mTagStackIndex < mTagStackSize,
+                 "Corrupt stackindex. Pointing outside stack!");

Why the change in nsHTMLContentSinkStream? (You know that it is used by the
browser? So, you better know it works.)

Comment 3

18 years ago
Did I mention that fixing this bug (and similar ones) is a good idea?
(Assignee)

Comment 4

18 years ago
The inclusion of nsHTMLContentSinkStream.cpp was a mistake. I forgot to edit the 
diff output. 

The reason I didn't bail out with a return was that this is an indication of 
major courruption and I thought that if I return we only would crash somewhere 
else. Maybe my reasoning is wrong.
Status: NEW → ASSIGNED

Comment 5

18 years ago
Note that assertions are only evaluated for DEBUG builds, so in production
builds, your test would do nothing at all. Returning with an error is safer
then, I think. If you want, you can do both.
if (mTagStackIndex >= mTagStackSize)
{
  NS_ASSERTION(PR_FALSE, "Corrupt stackindex: |mTagStackIndex >= mTagStackSize|.
Pointing outside stack!");
  return NS_ARRRGGG;
}
(Assignee)

Comment 6

18 years ago
Created attachment 15019 [details] [diff] [review]
Patch Mk.2
(Assignee)

Comment 7

18 years ago
I've attached a fix that I think is good. I've tested it now (had a corrupt 
component.reg before) and it works as I want. It reduces heap allocations by 
about 2 KB. The binary increases a couple of 100 bytes. 

I don't know though if this is something Netscape will want for NS6 so I think 
I'll wait till after they branched before checking it in. I will take rewiews 
though. :-)
Severity: normal → minor
Whiteboard: FIX IN HAND
Target Milestone: --- → M20
(Assignee)

Updated

18 years ago
Target Milestone: M20 → mozilla0.9
(Assignee)

Comment 8

18 years ago
I just want to say that one reason I haven't gone further with this is that the
decreased memory used is replaced with an increase in code size of almost the
same amount. I'm not sure if it's worthwhile.
 
(Assignee)

Comment 9

18 years ago
Are there no growable arrays in xpcom? Like std::vector<int> or something similar?
Whiteboard: FIX IN HAND
(Assignee)

Updated

18 years ago
Depends on: 38173
(Assignee)

Updated

18 years ago
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 10

18 years ago
bug 38173 is proposing ns[Auto]UInt32Array which might be useful here.
(Assignee)

Comment 11

18 years ago
Yes, that's why bug 38173 is listed as a blocker for this. I'm waiting... :-)

Comment 12

18 years ago
bug 38173 doesn't have a target milestone set. so I'm going to unset the
milestone on this one.  if we work out a schedule for getting the
dependency bug and this one fixed then lets retarget this bug
when we think it will land.

adding kandrot to the cc list.  maybe he can help on resovling the
dependency.

Updated

18 years ago
Target Milestone: mozilla0.9.1 → ---
(Assignee)

Comment 13

17 years ago
I have a new patch that uses the NS_PTR_TO_INT32 macro and an NS_PTR_TO_HTMLTAG
macro to get around the need for an int array by using the ordinary nsVoidArray.
The old patch was well, quite old anyway.

Harish, can you review it? The benefits are that we can take arbitary size input
while using 2KB less memory in the common case. 

jst, can you sr it?

Keywords: footprint
(Assignee)

Comment 14

17 years ago
Created attachment 60035 [details] [diff] [review]
Modern patch using nsVoidArray
Attachment #14885 - Attachment is obsolete: true
Attachment #15019 - Attachment is obsolete: true

Comment 15

16 years ago
Comment on attachment 60035 [details] [diff] [review]
Modern patch using nsVoidArray 

DOM folks, any stamps on the patch? Seems to look clean and good (if still
up-to-date).
(Assignee)

Comment 16

16 years ago
Created attachment 104606 [details] [diff] [review]
Resynched with the trunk

The attached patch is not mergeable but I have kept it up to date in my local
tree all the time so here is a usable patch.
Attachment #60035 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #104606 - Flags: superreview?(jst)
Attachment #104606 - Flags: review?(akkana)

Comment 17

16 years ago
Comment on attachment 104606 [details] [diff] [review]
Resynched with the trunk

Sorry, I spaced on the review request.

> #define NS_PTR_TO_HTMLTAG(x) NS_STATIC_CAST(nsHTMLTag, NS_PTR_TO_INT32(x))
> #define NS_HTMLTAG_TO_PTR(x) NS_INT32_TO_PTR(NS_STATIC_CAST(PRInt32, (x)))

Wow, I wish we had a class that didn't require this.  I guess what you're doing
is safe (you're storing integers representing html tags in void* spaces,
right?) and I don't know of another way using our classes, but ... ew.

The patch as is doesn't compile for me.  You need to add:
#include "nsVoidArray.h"
in nsPlainTextSerializer.h.

>+    PRInt32 nextToLastTagIndex = mTagStack.Count()-2;
>+    if (nextToLastTagIndex >= 0 && IsInOL()) {

nextToLastTagIndex is never used after this; wouldn't it be more
straightforward to just check whether mTagStack.Count() >= 2 and skip setting
the variable? 

r=akkana with the nsVoidArray.h include added (and the nextToLastTagIndex
change at your discretion).
Attachment #104606 - Flags: review?(akkana) → review+
(Assignee)

Comment 18

16 years ago
Yes, the nsVoidArray.h inclusion is needed since ParserService caching was
removed. I think that this patch might need an overhaul after bug 180996 is
fixed. The current patch there uses a stack that would be parallell to the tag
stack here. Those should probably be merged somehow, but right now I'm more
eager to get bug 180996 fixed and will deal with the consequences later.
(Assignee)

Comment 19

16 years ago
Comment on attachment 104606 [details] [diff] [review]
Resynched with the trunk

Canceling sr request. This patch will have to be updated after landing of some
other patches I prioritize higher than this.
Attachment #104606 - Flags: superreview?(jst)

Comment 20

16 years ago
Daniel, can this patch be brought up-to-date or are you still waiting on the
other patches?
(Assignee)

Comment 21

16 years ago
It could quite easily be brought up to date if I just hade some time to do it.  :-(


Daniel, are you still planning on working on this?
QA Contact: sujay → dom-to-text
You need to log in before you can comment on or make changes to this bug.