Bug 141247

Summary: REGRESSION (bmalloc): WebKit performance tests don't report memory stats
Product: WebKit Reporter: Carlos Alberto Lopez Perez <clopez>
Component: WebCore Misc.Assignee: Carlos Alberto Lopez Perez <clopez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cgarcia, cmarcelo, commit-queue, darin, dbates, ggaren, gyuyoung.kim, kling, kvserr, mitica, msaboff, olivier.blin, ossy, rniwa, sam, webkit-bug-importer, zan, zoltan
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 141459    
Attachments:
Description Flags
Patch none

Carlos Alberto Lopez Perez
Reported 2015-02-04 07:35:27 PST
Our performance tests, use the JS call window.internals.mallocStatistics() to get information about the internal malloc stats on WebKit. This information is collected by the tool running the performance tests and displayed on the graphs of http://perf.webkit.org This feature was added on r123773 <http://trac.webkit.org/r123773> (Check bug 78984) On bug 139812 I wondered why the Mac performance bots don't report any information about the malloc stats. It turns out the cause is that bmalloc still don't implements this. I just tested bmalloc on Linux (bug 140162) and I also get 0 bytes reported on the malloc stats of any performance test. For example, on the link below you can see how the mac bot stopped reporting any information about malloc stats once bmalloc was made the default allocator. Linux bots (GTK/EFL) still report this information because they are still using TCMalloc. https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22efl%22%2C%22Parser%2Fhtml5-full-render%3AMalloc%22%5D%2C%5B%22gtk%22%2C%22Parser%2Fhtml5-full-render%3AMalloc%22%5D%2C%5B%22mac-mavericks%22%2C%22Parser%2Fhtml5-full-render%3AMalloc%22%5D%2C%5B%22mac-yosemite%22%2C%22Parser%2Fhtml5-full-render%3AMalloc%22%5D%5D&days=351 I think this is a regression, since now we lack of proper information about the memory usage/consumption and we can't identify revisions that cause huge increases on the memory usage. The (stub) implementation for bmalloc seems to be there: http://trac.webkit.org/browser/trunk/Source/WTF/wtf/FastMalloc.cpp?rev=179600#L339 The one for tcmalloc that was working previously: http://trac.webkit.org/browser/trunk/Source/WTF/wtf/FastMalloc.cpp?rev=179600#L4552 I'm not planning to implement this, so feel free to take it if you want.
Attachments
Patch (2.40 KB, patch)
2015-07-03 10:28 PDT, Carlos Alberto Lopez Perez 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-02-04 20:22:30 PST
<rdar://problem/19726151>
Geoffrey Garen
Comment 2 2015-02-12 16:22:36 PST
It's not clear if we will ever get back to the precise reporting TCMalloc had; some of that reporting relied on expensive metadata that cause throughput and/or memory use problems. For example, TCMalloc could only do accurate reporting of committed vs uncommitted memory because it would synchronously over-commit huge swaths of memory, and synchronously commit or decommit when merging ranges, or refuse to merge ranges.
Csaba Osztrogonác
Comment 3 2015-02-13 05:06:41 PST
(In reply to comment #2) > It's not clear if we will ever get back to the precise reporting TCMalloc > had; some of that reporting relied on expensive metadata that cause > throughput and/or memory use problems. > > For example, TCMalloc could only do accurate reporting of committed vs > uncommitted memory because it would synchronously over-commit huge swaths of > memory, and synchronously commit or decommit when merging ranges, or refuse > to merge ranges. If it isn't possible to get back memory statistics, how can we avoid serious memory regressions in the future? How do you measure memory consumption on OSX since switching to bmalloc? Are you going to upstream your method to be able run memory measurements on the performance bots again?
Csaba Osztrogonác
Comment 4 2015-02-13 06:19:39 PST
I found bug136592 now, so it is a known issue long time ago.
Carlos Alberto Lopez Perez
Comment 5 2015-02-13 08:43:05 PST
As a workaround (until a better solution is found) I'm wondering if on Linux we could just use the information from the Kernel regarding memory usage. It should be easy to get both the resident and virtual memory usage from /proc/$pid/stat (rss and vsize fields) and report it. WDYT?
Sam Weinig
Comment 6 2015-02-13 09:30:01 PST
(In reply to comment #3) > (In reply to comment #2) > > It's not clear if we will ever get back to the precise reporting TCMalloc > > had; some of that reporting relied on expensive metadata that cause > > throughput and/or memory use problems. > > > > For example, TCMalloc could only do accurate reporting of committed vs > > uncommitted memory because it would synchronously over-commit huge swaths of > > memory, and synchronously commit or decommit when merging ranges, or refuse > > to merge ranges. > > If it isn't possible to get back memory statistics, how can we avoid serious > memory regressions in the future? How do you measure memory consumption on > OSX > since switching to bmalloc? Are you going to upstream your method to be able > run memory measurements on the performance bots again? We have never really relied on malloc statistics for our memory testing. On OS X we use the `footprint` tool, though in the past, we have used tools like `heap` and `vmmap`. These tools give better indications of memory use, as they include all of the allocators on the system.
Carlos Alberto Lopez Perez
Comment 7 2015-07-03 10:28:05 PDT
Created attachment 256108 [details] Patch
Darin Adler
Comment 8 2015-07-08 12:35:23 PDT
Geoff, are you going to review this?
Carlos Alberto Lopez Perez
Comment 9 2015-07-09 08:53:53 PDT
BTW, I have checked the output of the win EWS bot and the failure it gives looks totally unrelated with this bug, also it seems that it has built fine Source/WTF/wtf/FastMalloc.cpp that is the only file that this patch touches. python: can't open file '/home/buildbot/WebKit/WebKitBuild/Release/include/private/JavaScriptCore/cssmin.py': [Errno 2] No such file or directory make: *** No rule to make target '/home/buildbot/WebKit/WebKitBuild/Release/include/private/JavaScriptCore/JSInputs.json', needed by 'WebReplayInputs.h'. Stop. make: *** Waiting for unfinished jobs.... /home/buildbot/WebKit/Source/WebCore/DerivedSources.make:893: recipe for target 'XMLViewerCSS.h' failed make: *** [XMLViewerCSS.h] Error 2 So I deduce that the win EWS is in bad status, checking https://webkit-queues.appspot.com/queue-status/win-ews seems to confirm that as is failing for every patch or not being able to build without patch.
Csaba Osztrogonác
Comment 10 2015-07-09 09:13:07 PDT
(In reply to comment #9) > BTW, I have checked the output of the win EWS bot and the failure it gives > looks totally unrelated with this bug, also it seems that it has built fine > Source/WTF/wtf/FastMalloc.cpp that is the only file that this patch touches. > > > python: can't open file > '/home/buildbot/WebKit/WebKitBuild/Release/include/private/JavaScriptCore/ > cssmin.py': [Errno 2] No such file or directory > make: *** No rule to make target > '/home/buildbot/WebKit/WebKitBuild/Release/include/private/JavaScriptCore/ > JSInputs.json', needed by 'WebReplayInputs.h'. Stop. > make: *** Waiting for unfinished jobs.... > /home/buildbot/WebKit/Source/WebCore/DerivedSources.make:893: recipe for > target 'XMLViewerCSS.h' failed > make: *** [XMLViewerCSS.h] Error 2 > > > So I deduce that the win EWS is in bad status, checking > https://webkit-queues.appspot.com/queue-status/win-ews seems to confirm that > as is failing for every patch or not being able to build without patch. Clean build is broken on Windows, it isn't an EWS issue. I already reported it in bug146628 and pinged Apple Windows port maintainers too in email. They know the problem, maybe they can fix it next week.
Zoltan Horvath
Comment 11 2015-07-14 15:15:26 PDT
By using TCmalloc's statistics, we intended to log memory allocated by new. Since there is no immediate solution to get stats from bmalloc, I think it's fine to save the max rss for mac. I'm not sure linux kernel will provide the same result for maxrss though.
Carlos Alberto Lopez Perez
Comment 12 2015-07-16 03:06:31 PDT
(In reply to comment #11) > By using TCmalloc's statistics, we intended to log memory allocated by new. > Since there is no immediate solution to get stats from bmalloc, I think it's > fine to save the max rss for mac. I'm not sure linux kernel will provide the > same result for maxrss though. What are your concerns regarding the Linux kernel? On the tests I did, the results I got with this patch where the ones I was expecting.
Zoltan Horvath
Comment 13 2015-07-17 13:18:45 PDT
(In reply to comment #12) > (In reply to comment #11) > > By using TCmalloc's statistics, we intended to log memory allocated by new. > > Since there is no immediate solution to get stats from bmalloc, I think it's > > fine to save the max rss for mac. I'm not sure linux kernel will provide the > > same result for maxrss though. > > What are your concerns regarding the Linux kernel? On the tests I did, the > results I got with this patch where the ones I was expecting. Good to hear that! Just wanted to be sure it provides the numbers that you want to see.
Carlos Alberto Lopez Perez
Comment 14 2015-07-21 16:13:45 PDT
(In reply to comment #8) > Geoff, are you going to review this? Not sure about Geoff... but is there any other reviewer willing to review this patch? It has been waiting here for review for almost 3 weeks already. I think that this patch is easy to review and brings back a missing functionality (since bmalloc) that is very useful (at least for me) to track down regressions in memory usage at http://perf.webkit.org Thanks.
Geoffrey Garen
Comment 15 2015-07-24 15:45:39 PDT
Comment on attachment 256108 [details] Patch These days, bmalloc has the metadata you need to produce numbers for reservedVMBytes and committedVMBytes. It's not so great to report overall memory usage like this, since that counts non-malloc memory. Still, I guess it's not actively harmful, and it will get *some* number back up on the bots, so I'll say r+.
Carlos Alberto Lopez Perez
Comment 16 2015-07-25 07:46:48 PDT
(In reply to comment #15) > Comment on attachment 256108 [details] > Patch > > These days, bmalloc has the metadata you need to produce numbers for > reservedVMBytes and committedVMBytes. > > It's not so great to report overall memory usage like this, since that > counts non-malloc memory. > > Still, I guess it's not actively harmful, and it will get *some* number back > up on the bots, so I'll say r+. I agree with you that there should be a better way to report the memory usage than this, but in the meanwhile at least this get us some stats back. Thanks for the r+ :)
Carlos Alberto Lopez Perez
Comment 17 2015-07-25 07:47:45 PDT
Comment on attachment 256108 [details] Patch Clearing flags on attachment: 256108 Committed r187389: <http://trac.webkit.org/changeset/187389>
Carlos Alberto Lopez Perez
Comment 18 2015-07-25 07:47:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.