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

Status: RESOLVED INACTIVE

(Core :: DOM: Serializers, defect, P3)

Description

[Daniel Bratell]

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.

Comment 1

[Daniel Bratell]

Attachment: Patch to reduce memory usage.

Comment 2

[Ben Bucksch (:BenB)]

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

[Ben Bucksch (:BenB)]

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

Comment 4

[Daniel Bratell]

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.

Comment 5

[Ben Bucksch (:BenB)]

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;
}

Comment 6

[Daniel Bratell]

Attachment: Patch Mk.2

Comment 7

[Daniel Bratell]

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. :-)

Comment 8

[Daniel Bratell]

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.
 

Comment 9

[Daniel Bratell]

Are there no growable arrays in xpcom? Like std::vector<int> or something similar?

Comment 10

[rbs]

bug 38173 is proposing ns[Auto]UInt32Array which might be useful here.

Comment 11

[Daniel Bratell]

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

Comment 12

[chris hofmann]

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.

Comment 13

[Daniel Bratell]

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?

Comment 14

[Daniel Bratell]

Attachment: Modern patch using nsVoidArray

Comment 15

[rbs]

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).

Comment 16

[Daniel Bratell]

Attachment: 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.

Comment 17

[Akkana Peck]

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).

Comment 18

[Daniel Bratell]

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.

Comment 19

[Daniel Bratell]

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.

Comment 20

[Stephen Walker]

Daniel, can this patch be brought up-to-date or are you still waiting on the
other patches?

Comment 21

[Daniel Bratell]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Daniel, are you still planning on working on this?

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.