Open Bug 116023 Opened 23 years ago Updated 2 years ago

1MB memory churned by nsParser::OnDataAvailable() calls ParserWriteFunc

Categories

(Core :: XML, defect, P3)

defect

Tracking

()

People

(Reporter: dp, Unassigned)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file, 3 obsolete files)

nsParser::OnDataAvailable() calls nsScanner::Appened() which ends up Allocating
memory and free it eventually.
This is at startup of browser with default profile and mozilla.org start page.

Wondering if we can do some buffer mgmt and reuse
Attached file One of the traces from spacetrace (obsolete) —
OnDataAvailable() calls ParserWriteFunc() which is a static. Spacetrace cant
report statics and hence is reporting nsScanner::Append().
Status: NEW → ASSIGNED
Summary: 1MB memory churned by nsParser::OnDataAvailable() calls nsScanner::Append*() → 1MB memory churned by nsParser::OnDataAvailable() calls ParserWriteFunc
[ 214 ] -  parser-nsScanner::Append  -  1,045,578
[  70 ] -  parser-creating-scanner   -      2,800

nsSlidingString::AppendBuffer() does one more allocation per call
Post our (Harish & I) discussion:

All this sliding string doesn't make sense. There is no need for the parser to
keep the incomming bytes, convert it into unicode and so all this sliding string
stuff for xml post harish's nsExpatDriver and removal of Tokenizer for xml.

Harish thinks he can remove all this 1MB of churn altogether. yeah!
Assignee: dp → harishd
Status: ASSIGNED → NEW
Keywords: perf
>All this sliding string doesn't make sense. 

From our discussion I understand that bypassing sliding string for xml data
would avoid additional mallocs.

>There is no need for the parser to keep the incomming bytes, convert it into
>unicode

Apparently unicode coversion is necessary.

Will have to figure out if this is really possible.

Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.0
Blocks: 92580
I almost have a patch to fix this. Taking over bug.
Assignee: harishd → dp
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Keywords: nsbeta1
*** Bug 125279 has been marked as a duplicate of this bug. ***
Depends on: 119631
Target Milestone: mozilla1.0 → mozilla0.9.9
Some notes:

- Patch depends the sliding string to accept a freeproc
- Emperical results show we only need 7 buckets to satisfy startup
- Maybe we should move RawToUnicode() to strings as CopyRawToUnicode()
Jag this is the where the sliding string free proc gets used to recycle buffers.
Startup: improved by 1.5%

Max Vmusage: reduced by 2.5%
             56,472 -> 55,068
             => about 1.4MB (2.5%)

Vmusage at end of startup : 52,828 in both cases
Comment on attachment 69269 [details] [diff] [review]
Parser using nsRecyclingAllocator

Assert / bullet proof in RawToUnicode() if aStr or aDest is null. With that
r=harishd
Attachment #69269 - Flags: review+
Simon sr= ?
-    char* buf = new char[kBufsize+1];
+    char* buf[kBufsize+1];

Do you really mean this? Or do you mean:

-    char* buf = new char[kBufsize+1];
+    char  buf[kBufsize+1];


+      PRUnichar *unichars = (PRUnichar*) nsScannerAllocate((numread+1) * 
sizeof(PRUnichar));
+      NS_ENSURE_TRUE(unichars, NS_ERROR_OUT_OF_MEMORY);
+      RawToUnicode((const char *)buf, numread, unichars, numread+1);
       AppendToBuffer(unichars, unichars+numread, unichars+kBufsize+1);

Why don't we just write a version of 'AppendToBuffer' that does the inflation?
Oops. Meant char buf[kBufSize+1]; Fixed that.

If we wrote AppendToBuffer that does the conversion, then we will need two: one
that takes PRUnichars and another with char *. I will do that.
Attached patch Per simon's comments (obsolete) — Splinter Review
Made a new AppendToBuffer() that will do the conversion. Fixed char* buf[] to
char buf[].
Attachment #69269 - Attachment is obsolete: true
I was hoping that having a char* version of AppendToBuffer would eliminate the
need to allocate, expand and copy to put the data into mSlidingBuffer. However,
it  seems that this is not the case, and mSlidingBuffer's APIs make this difficult.

+void nsScanner::AppendToBuffer(const nsAString &aData)
+{
+  PRUint32 len = aData.Length();
+  PRUnichar* buffer = (PRUnichar *) nsScannerAllocate(len * sizeof(PRUnichar));
+  if (buffer) {
+    CopyUnicodeTo(aData, 0, buffer, len);
+    AppendToBuffer(buffer, buffer+len, buffer+len);
+  }

Isn't this where we'd use nsPromiseFlatString? If aData is already flat, this
avoids an extra allocate and copy.
The AppendToBuffer(nsAString) version gets called so less. I will put in numbers
on how often that gets called. Harish ?
14 out of 202 after startup
218 out of 1276 post running pageload test

Anyway, let me see if we can use nsPromiseFlatString.
We cant use PromiseFlatString here: the reason being there is no lifetime
gurantees as to the buffer and the nsAString in question.

I was looking if there was a way to get the buffer out of the nsAString, have it
disown the buffer, use the buffer (no allocation). PromiseFlatString isnt that
and I dont think this is possible. Let me ask jag just in case.
Comment on attachment 71571 [details] [diff] [review]
Per simon's comments

sr=sfraser, and bringing review forward
Attachment #71571 - Flags: superreview+
Attachment #71571 - Flags: review+
dp is right, since this string needs to own the result, you just need to copy.
Comment on attachment 71571 [details] [diff] [review]
Per simon's comments

a=asa (on behalf of drivers) for checkin to 0.9.9 and the trunk.
Attachment #71571 - Flags: approval+
This was backed out since it caused the DomToTextConversionTest 
(TestOutSinks.pl) test to start failing, which turned comet and 
sleestack orange. 

Oddly, the test did not fail on a few occasions on comet, and also
did not fail on my own Linux tip build. Which is a pain.
Tried running it through Purify? Sounds a little like an Uninitialized Memory Read.
From staring at the patch there is one semantic change that this introduces:

nsScanner::nsScanner(nsAString& aHTMLString,...)

This function used to do:
PRUnichar *buffer = ToNewUnicode(aHTMLString);
AppendToBuffer(buffer, buffer+aHTMLString.Length(),...)

When aHTMLString is empty, this still returned an allocated buffer with 1 null
character. Length() was still 0. So there was this initial AppendToBuffer() call
happening.

With the patch, if length was 0, then no AppendToBuffer is done.

The test uses this type of constructor while mozilla doesnt.

I will look into the effects of this in a bit. I will have to backout my change
from branch too...
BTW, the tinderbox leak stats on ... went up from 4.84KB to 4.90KB caused by
this checkin:
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1015107900&maxdate=1015112219

This doesn't necessarily indicate a problem with this patch, though.
Oops, I wanted to say the stats on tinderbox "brad" went up. (But on shrike,
they went up, too, from Lk:5.19KB to Lk:5.27KB.) 
Oh yeah. The allocator needs to be released too.

The issue here is the tests use a constructor that there is some code somewhere
that is missing a if (!mSlidingBuffer) return; protection. Will figure that out.
Also will need to release the allocator.

I will reset r/sr/a on this bug and will go through the process again.
Comment on attachment 71571 [details] [diff] [review]
Per simon's comments

Resetting r/sr/a

Needs work: sleestack failed DOMToTestConversionTest. Also need to release
allocator.
Attachment #71571 - Flags: superreview+
Attachment #71571 - Flags: review+
Attachment #71571 - Flags: needs-work+
Attachment #71571 - Flags: approval+
- Fixes TestOutSink.pl hang. At one point in code, mCurrentPosition was
accessed without checking mSlidingBuffer. Plugged that.

- Delete nsScanner::gAllocator on module shutdown
Attachment #62219 - Attachment is obsolete: true
Attachment #71571 - Attachment is obsolete: true
I tested the above on sleestack to make sure we pass the tests.

harishd/simon : r=/sr=
+  mTotalRead = anHTMLString.Length();
+  if (mTotalRead) {
+    // Append to buffer will take care of initializing mCurrentPosition
+    // and mMarkPosition
+    AppendToBuffer(anHTMLString);
+  }

When anHTMLString is empty mCurrentPostion will be unintialized. no?
 PRBool nsScanner::UngetReadable(const nsAReadableString& aBuffer) {
-  mSlidingBuffer->UngetReadable(aBuffer,mCurrentPosition);
-  mSlidingBuffer->BeginReading(mCurrentPosition); // Insertion invalidated our
iterators
-  mSlidingBuffer->EndReading(mEndPosition);
+  if (!mSlidingBuffer) {
+    // This happens to be the first buffer. Just append it.
+    AppendToBuffer(aBuffer);
+  }

When does this happen? 
Comment on attachment 72683 [details] [diff] [review]
Fixes TestOutSink.pl hang and delete of allocator

r=harishd ( talked to dp over the phone ).
Attachment #72683 - Flags: review+
For the record: mCurrentPosition and mMarkPosition will be in an invalid state
post nsScanner::nsScanner in most cases (with and without patch). Most of the
rest of code protects itself by checking if (mSlidingbuffer). The check was
missing from one place. The old patch caused no AppendToBuffer() to happen for
empty strings passed into constructor. This coupled with the check missing
causes the test to fail. Mozilla didn't encounter that case.

The patch fixes that hole of not checking mSlidingBuffer before accessing
mCurrentPositiona nd mMarkPosition in every possible case.
+    delete nsScanner::gAllocator;
You should probably set gAllocator to nsnull here, just in case.

+  while (*aStr)
+    *aDest++ = (unsigned char) *aStr++;

Why isn't this a PRUnichar cast?

Fix those, and sr=sfraser
> +  while (*aStr)
> +    *aDest++ = (unsigned char) *aStr++;
>
> Why isn't this a PRUnichar cast?

That would be dangerous. *aStr is a char * (signed maybe). So making it unsigned
causes no sign extension to happen when it eventually gets converted to
PRUnichar. So what the patch is doing effectively is:

*aDest++ = (PRUnichar) (unsigned char) *aStr++;

Converting directly to an unsigned int might cause sign extension!!! 

Sample program to test theory out:

g> cat a.c
#include <stdio.h>

main()
{
        signed char s = 0xF5;
        unsigned int x = s;
        unsigned int y = (unsigned char) s;
        unsigned int z = (unsigned int) s;

        printf("x = %d, y = %d, z = %d\n", x, y, z);
        return 0;
}
g> ./a
x = -11, y = 245, z = -11

dp, you are correct, it's the same cast we needed in our string code. I think
what smfr is making clear though is that the code as-is is confusing, which is
why I would prefer spelling out the implied conversion:

+  while (*aStr)
+    *aDest++ = (PRUnichar) (unsigned char) *aStr++;


Or:

+  while (*aStr)
+    *aDest++ = PRUnichar((unsigned char)*aStr++);

Whichever you find more appealing.
Comment on attachment 72683 [details] [diff] [review]
Fixes TestOutSink.pl hang and delete of allocator

Simon sr= ed. Will fix both.
Attachment #72683 - Flags: superreview+
Comment on attachment 72683 [details] [diff] [review]
Fixes TestOutSink.pl hang and delete of allocator

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72683 - Flags: approval+
+ing.
Keywords: nsbeta1nsbeta1+
So here is whats happening on the failing test TestOutSink.pl:

Test passes but hangs on shutdown. Reason is the timer that
nsParser->nsScanner->nsRecyclingAllocator is initiating. This is reproducible
only on comet.mcom.com and happens randomly.

I am digging deeper to see whats going wrong with the timer thread. (Ccing pav)
has 72683 landed? can you obsolete that patch if it has so that it doesn't look
like an approved change that hasn't landed yet. If it hasn't landed yet can you
remove the approval and re-request from drivers? thanks.
The patch hasn't landed. I am leaning on not landing this for mozilla 1.0 (this
is a pref thing not footprint and more timers close to a release isn't a good
thing) 

The value of this is this could be showing us a timer problem. I will bring this
back to radar for the timer issue for mozilla 1.0 as a separate bug.
Target Milestone: mozilla0.9.9 → mozilla1.0.1
Comment on attachment 72683 [details] [diff] [review]
Fixes TestOutSink.pl hang and delete of allocator

Removing r/sr/a.
Attachment #72683 - Flags: superreview+
Attachment #72683 - Flags: review+
Attachment #72683 - Flags: approval+
What's the current status of this one?
Is this still planned for the future, or is it a lost cause ?
Is this still something to pursue?
(In reply to comment #48)
> Is this still planned for the future, or is it a lost cause ?

  Alfred Kayser in comment #49
> Is this still something to pursue?
Assignee: dp → nobody
Status: ASSIGNED → NEW
Keywords: qawanted
QA Contact: moied → parser
Whiteboard: [MemShrink]
Is this even relevant anymore with the HTML5 parser?
OS: Windows 2000 → All
Hardware: x86 → All
Target Milestone: mozilla1.0.1 → ---
I don't know if this precise problem still exists, but nsScanner is still used for XML parsing (unfortunately; bug 651045 depends on my planned by not executed XML parsing rewrite).

Proxying the stream data to the parser thread in the new HTML parser ends up using malloc for all stream data. However, that's what malloc is for, though.
Whiteboard: [MemShrink]
(In reply to Wayne Mery (:wsmwk) from comment #50)
What's needed from qa here ?
(In reply to Paul Silaghi [QA] from comment #53)
> (In reply to Wayne Mery (:wsmwk) from comment #50)
> What's needed from qa here ?

Hi Paul.  Perhaps nothing is needed, given the subsequent feedback of comment 51 and comment 52.  Unless you want to weigh in on the questions.  n.b. they are not my questions.
Thanks Wayne.
Dropping qawanted based on comment 54.
Keywords: qawanted
Component: HTML: Parser → XML
Moving to p3 because no activity for at least 24 weeks.
Priority: P1 → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: