Closed
Bug 177994
Opened 22 years ago
Closed 22 years ago
Do not hold parser nodes longer than necessary
Categories
(Core :: DOM: HTML Parser, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: harishd, Assigned: harishd)
Details
(Keywords: memory-footprint)
Attachments
(2 files, 2 obsolete files)
|
98.61 KB,
patch
|
Details | Diff | Splinter Review | |
|
116.79 KB,
patch
|
hjtoi-bugzilla
:
review+
harishd
:
superreview+
|
Details | Diff | Splinter Review |
Except nodes ( parser nodes ) that carry residual style information all other
nodes can be recycled immediately after processing ( by NavDTD and ContentSink ).
This patch does the following:
1) Recycle parser nodes, that do not carry residual style info., immediately
after processing
2) Split parser node into two - nsCParserNode and nsCParserStartNode. This
split will reduce the size of nsCParserNodes because it does not have to store
attribute information unlike nsCParserStartNode.
3) Reduce initial-node-pool-size - Since the parser nodes are recycled faster
we have the luxury to reduce the pool size :-)
Comment on attachment 104919 [details] [diff] [review]
Patch v1.0
>Index: htmlparser/public/nsIHTMLContentSink.h
>===================================================================
> * @param nsIParserNode reference to parser node interface
> */
>- NS_IMETHOD CloseHTML(const nsIParserNode& aNode)=0;
>+ NS_IMETHOD CloseHTML() = 0;
There is no longer paramaters to this function so comments are wrong. Check the
other functions for this pattern as well.
> * @param nsIParserNode reference to parser node interface
> */
>- NS_IMETHOD CloseContainer(const nsIParserNode& aNode) = 0;
>+ NS_IMETHOD CloseContainer(const nsHTMLTag aTag) = 0;
Different parameter, fix comment.
>Index: htmlparser/public/nsIParserNode.h
>===================================================================
> * @param anIndex is the index of the key you want
> * @return string containing key.
> */
>- virtual const nsAString& GetKeyAt(PRUint32 anIndex) const =0;
>+ virtual const nsAString& GetNameAt(PRUint32 anIndex) const = 0;
Comment does not match, new code talks about names.
Will continue...
Attachment #104919 -
Flags: superreview?(jst)
Attachment #104919 -
Flags: review?(heikki)
Attachment #104919 -
Attachment is obsolete: true
Attachment #104919 -
Flags: superreview?(jst)
Attachment #104919 -
Flags: review?(heikki)
Attachment #105893 -
Flags: superreview?(jst)
Attachment #105893 -
Flags: review?(heikki)
|
||
Comment 5•22 years ago
|
||
Comment on attachment 105893 [details] [diff] [review]
patch v1.1
+CNavDTD::OpenContainer(const nsCParserNode *aNode,
+ eHTMLTags aTag,PRBool
+ aClosedByStartTag,
Push the type of aClosedByStartTag type (PRBool) down to the same line where
the argument is.
@@ -3618,25 +3570,24 @@
2. <body><a>text<a> //in this case, the target matches, so
don't push style
***************************************************************************/
- if(0==theNode->mUseCount){
- if(theTag!=aTarget) {
+ if (theNode->mUseCount){
Didn't you mean !theNode->mUseCount (or theNode->mUseCount == 0) here?
- In htmlparser/src/nsElementTable.cpp:
-TagList
gHeadKids={9,{eHTMLTag_base,eHTMLTag_bgsound,eHTMLTag_link,eHTMLTag_meta,eHTMLT
ag_script,eHTMLTag_style,eHTMLTag_title,eHTMLTag_noembed,eHTMLTag_userdefined}}
; // added userdefined to fix bug 167493
+TagList
gHeadKids={8,{eHTMLTag_base,eHTMLTag_bgsound,eHTMLTag_link,eHTMLTag_meta,eHTMLT
ag_script,eHTMLTag_style,eHTMLTag_title,eHTMLTag_noembed}};
This looks like a change unrelated to the problem you're fixing here, or did I
miss something?
- In nsParserNode.cpp:
nsresult nsCParserNode::ReleaseAll() {
- if(mAttributes) {
- NS_ASSERTION(0!=mTokenAllocator, "Error: no token allocator");
- if(mTokenAllocator) {
- CToken* theAttrToken=0;
- while((theAttrToken=NS_STATIC_CAST(CToken*,mAttributes->Pop()))) {
+ if(mTokenAllocator) {
+ IF_FREE(mToken,mTokenAllocator);
+ }
+ return NS_OK;
+}
+
+nsresult
+nsCParserStartNode::Init(CToken* aToken,
+ nsTokenAllocator* aTokenAllocator,
+ nsNodeAllocator* aNodeAllocator)
+{
+ if (mAttributes.GetSize()) {
+ NS_ASSERTION(0 != mTokenAllocator, "Error: Attribute tokens on node
without token allocator");
+ if (mTokenAllocator) {
+ CToken* theAttrToken = 0;
+ while ((theAttrToken = NS_STATIC_CAST(CToken*,mAttributes.Pop()))) {
IF_FREE(theAttrToken, mTokenAllocator);
}
}
- delete mAttributes;
- mAttributes=0;
}
+ return nsCParserNode::Init(aToken, aTokenAllocator, aNodeAllocator);
+}
This looks odd to me. Are you sure you want to delay recycling (or releasing)
of attributes until a start node is re-initialized? Shouldn't that always
happen sooner?
- In nsParserNode.h:
+ nsDeque mAttributes;
I would almost be willing to bet money on that this could be made faster (and
cleaner) with an nsAutoVoidArray.
- In nsPlainTextSerializer::CloseContainer():
+ // XXX What do we need this?
s/What/Why/ or add "for" at the end...
+ // mParserNode = NS_CONST_CAST(nsIParserNode*, &aNode);
But then again we should really figure out why this code was added here before
checking in. If this is needed, then we could write this code to use a stack of
parser nodes in stead of relying on being passed the parser node when a tag is
closed.
- In nsHTMLContentSink.cpp:
+ //SINK_TRACE_NODE(SINK_TRACE_CALLS,
+ // "SinkContext::CloseContainer", aNode, mStackPos - 1,
mSink);
Do you really want to just comment this out? Is there an alternative to this
code that we need now that we don't have a parser node when closing a
container?
There's a bunch of these commented out...
Other than that, sr=jst
Attachment #105893 -
Flags: superreview?(jst) → superreview+
>- if(0==theNode->mUseCount){
>- if(theTag!=aTarget) {
>+ if (theNode->mUseCount){
>Didn't you mean !theNode->mUseCount (or theNode->mUseCount == 0) here?
Good catch. Yes it should be theNode->mUseCount == 0
>-TagList
>gHeadKids={9,{eHTMLTag_base,eHTMLTag_bgsound,eHTMLTag_link,eHTMLTag_meta,eHTMLT
>ag_script,eHTMLTag_style,eHTMLTag_title,eHTMLTag_noembed,eHTMLTag_userdefined}}
>; // added userdefined to fix bug 167493
>+TagList
>gHeadKids={8,{eHTMLTag_base,eHTMLTag_bgsound,eHTMLTag_link,eHTMLTag_meta,eHTMLT
>ag_script,eHTMLTag_style,eHTMLTag_title,eHTMLTag_noembed}};
>This looks like a change unrelated to the problem you're fixing here, or did I
miss something?
Yes. I'll remove this change from the patch.
>+ // XXX Why is this here?
>+ // mParserNode = NS_CONST_CAST(nsIParserNode*, &aNode);
>But then again we should really figure out why this code was added here before
>checking in. If this is needed, then we could write this code to use a stack of
>parser nodes in stead of relying on being passed the parser node when a tag is
>closed.
AFAICT, there should be no need for mParserNode in ::CloseContainer.
>+ //SINK_TRACE_NODE(SINK_TRACE_CALLS,
>+ // "SinkContext::CloseContainer", aNode, mStackPos - 1,
>mSink);
>Do you really want to just comment this out? Is there an alternative to this
>code that we need now that we don't have a parser node when closing a
>container?
I'll tweak the macro accordingly
Addresses issues ( most of 'em ) raised by jst.
Note: Did not replace nsDeque with nsAutoVoidArray.
Attachment #105893 -
Attachment is obsolete: true
Comment on attachment 106398 [details] [diff] [review]
patch v1.2
Carried forward jst's sr=.
Attachment #106398 -
Flags: superreview+
Attachment #106398 -
Flags: review?(heikki)
|
||
Updated•22 years ago
|
Attachment #105893 -
Flags: review?(heikki) → review-
Comment on attachment 106398 [details] [diff] [review]
patch v1.2
You should be consistent with whitespace, but ok...
Minor nit in nsCParserStartNode::GetSource(): you can skip |aString.Truncate()|
and do |aString.Assign(PRUnichar('<'))| instead. The function call in the
for-loop in that function does not look too nice either, btw, could it be
avoided?
That's all I could find this time.
Attachment #106398 -
Flags: review?(heikki) → review+
|
||
Comment 10•22 years ago
|
||
harish, did you intend to check in the change to
mozilla/htmlparser/tests/html/142965.html?
|
Assignee | |
Comment 11•22 years ago
|
||
> mozilla/htmlparser/tests/html/142965.html?
No. But it shouldn't hurt. right?
|
|
Assignee | |
Comment 12•22 years ago
|
||
doh....thought I added a new file 42965.html but apparently I modified it!. Will
back it out. Sorry
|
||
Comment 13•22 years ago
|
||
Oops, forgot to post our footprint data in the bug. :-)
Here are the footprint and perf data that harishd and I saw with his patch:
Baseline/Original Build:
-------------------------
Peak Memory: 14,977,304 bytes
Total Calls: 2,577,811 [ 2,325,968 + 217,320 + 34,523 ]
Patch
-------------------------
Peak Memory: 14,462,920 bytes
Total Calls: 2,350,334 [ 2,187,016 + 129,408 + 33,910 ]
patch + arena at 25
-------------------------
Peak Memory: 14,782,488 bytes
Total Calls: 2,319,431 [ 2,159,241 + 129,155 + 31,035 ]
patch + arena at 35 (reflects final patch)
-------------------------
Peak Memory: 14,156,320 bytes
Total Calls: 2,313,557 [ 2,154,209 + 128, 540 + 30,808 ]
** Tatal Calls are malloc + calloc + realloc. **
Pageload and VM data:
Original patch patch + *25 patch + *35
--------- --------- ---------- ---------
pageload: 4489 ms 4493 ms 4521 ms 4473 ms
data KB: 19,338 kb 19,152 kb 18,952 kb 18,584 kb
code KB: 15,220 kb 15,192 kb 15,220 kb 15,192 kb
Harish's patch (with arena set to 35) is the most optimal win.
|
||
Comment 14•22 years ago
|
||
It seems that the checkins for this bug have caused regression discussed in bug
138125.
|
||
Comment 15•22 years ago
|
||
Bug 182021 filed for that regression.
|
|
Assignee | |
Comment 16•22 years ago
|
||
Fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•