Bug 106586 - NodeRareData and NodeListsNodeData are never deleted until Node is deleted even if they're no longer used
Summary: NodeRareData and NodeListsNodeData are never deleted until Node is deleted ev...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 79740 107074
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-10 11:15 PST by Ryosuke Niwa
Modified: 2019-08-14 16:59 PDT (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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.