Bug 106586

Summary: NodeRareData and NodeListsNodeData are never deleted until Node is deleted even if they're no longer used
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: barraclough, darin, esprehn, koivisto, mjs, morrita, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 79740, 107074    
Bug Blocks:    

Description Ryosuke Niwa 2013-01-10 11:15:35 PST
Right now, we never delete NodeRareData and NodeListsNodeData once they’re created. This results in some pathological memory use when the script traverse the entire document using node.childNodes for example because we’ll end up creating childNodes on all nodes temporarily and that results in every node having NodeRareData and NodeListsNodeData.
Comment 1 Radar WebKit Bug Importer 2013-01-10 11:16:25 PST
<rdar://problem/12990725>
Comment 2 Ryosuke Niwa 2013-01-10 11:19:13 PST
Also see the bug 79740.
Comment 3 Elliott Sprehn 2013-01-10 12:30:33 PST
Note that they _are_ deleted, but not until the Node is. The description and title on this bug make it sound like we're leaking memory when we aren't.
Comment 4 Ryosuke Niwa 2013-01-10 12:34:33 PST
(In reply to comment #3)
> Note that they _are_ deleted, but not until the Node is. The description and title on this bug make it sound like we're leaking memory when we aren't.

That's a good point. Renamed.
Comment 5 Elliott Sprehn 2013-01-16 17:53:09 PST
I have some fear this will regress performance by creating a lot of memory churn. For instance doing node.childList repeatedly and letting it drop off it will not just allocate the ChildNodeList like it does now, but the NodeRareData, NodeListsNodeData and the ChildNodeList every time.
Comment 6 Ryosuke Niwa 2013-01-16 17:55:50 PST
(In reply to comment #5)
> I have some fear this will regress performance by creating a lot of memory churn. For instance doing node.childList repeatedly and letting it drop off it will not just allocate the ChildNodeList like it does now, but the NodeRareData, NodeListsNodeData and the ChildNodeList every time.

That should be considered as a bug/performance issue in GC. A good GC shouldn't be dropping childNodes repeatedly like that.
Comment 7 Elliott Sprehn 2013-01-24 20:37:58 PST
(In reply to comment #6)
> (In reply to comment #5)
> > I have some fear this will regress performance by creating a lot of memory churn. For instance doing node.childList repeatedly and letting it drop off it will not just allocate the ChildNodeList like it does now, but the NodeRareData, NodeListsNodeData and the ChildNodeList every time.
> 
> That should be considered as a bug/performance issue in GC. A good GC shouldn't be dropping childNodes repeatedly like that.

See the discussion on https://bugs.webkit.org/show_bug.cgi?id=106726

It appears that once you have rare data childNodes is noticeably faster, so dropping rare data when the last node list goes away will probably make repeated traversals of the DOM through childNodes slower. I also question the perf impact from adding all of these subobjects into rare data, or dropping them, since it means lots more little allocations.

I'd really like to see some numbers of real world rare data usage and if this is worth it. It doesn't appear this is going to be much of a memory savings, maybe a few KB, and it has a known perf impact now.
Comment 8 Ryosuke Niwa 2013-01-24 20:46:02 PST
(In reply to comment #7)
> It appears that once you have rare data childNodes is noticeably faster, so dropping rare data when the last node list goes away will probably make repeated traversals of the DOM through childNodes slower. I also question the perf impact from adding all of these subobjects into rare data, or dropping them, since it means lots more little allocations.

Of course, it'll make things slower because it's a trade off between CPU and memory usage. But did you see any perf. regressions from https://bugs.webkit.org/show_bug.cgi?id=107074 ? If not, I highly doubt that deleting one more object will have a significant perf. impact. And 3% memory use reduction is quite significant.
Comment 9 Elliott Sprehn 2013-01-24 21:14:51 PST
(In reply to comment #8)
> (In reply to comment #7)
> > It appears that once you have rare data childNodes is noticeably faster, so dropping rare data when the last node list goes away will probably make repeated traversals of the DOM through childNodes slower. I also question the perf impact from adding all of these subobjects into rare data, or dropping them, since it means lots more little allocations.
> 
> Of course, it'll make things slower because it's a trade off between CPU and memory usage. But did you see any perf. regressions from https://bugs.webkit.org/show_bug.cgi?id=107074 ? If not, I highly doubt that deleting one more object will have a significant perf. impact. And 3% memory use reduction is quite significant.

Can you explain the 3% improvement? When I looked into this there were about 3000 elements in a large sized web app, if at most 10% have rare data and rare data is 160 bytes (that was months ago, it's smaller now) that's only 47KB. That seems a far cry from 3% of the memory usage of a typical web app.

Can you post some real numbers of rare data usage on pages and how much memory this really saves? It would make sense to instrument rare data usage before we start these kinds of optimizations.
Comment 10 Maciej Stachowiak 2013-02-15 19:23:00 PST
It seems like it would be easier to evaluate this with a patch that can be read and tested, plus some test data, than in the abstract.