Bug 186698

Summary: Make it possible to track call sites that waste Vector and HashMap capacity
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Tools / TestsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: NEW    
Severity: Normal CC: annulen, ap, benjamin, cdumez, changseok, cmarcelo, darin, dbates, dino, ews-watchlist, fmalita, fred.wang, gyuyoung.kim, joepeck, lforschler, mmaxfield, pdr, rbuis, ryuan.choi, saam, sabouhallawa, sam, schenney, sergio, simon.fraser, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Track Vector and HashTable capacity
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Simon Fraser (smfr)
Reported 2018-06-15 14:57:37 PDT
We should have tooling that we can enable that allows us to find call sites that waste vector capacity.
Attachments
Patch (43.27 KB, patch)
2018-06-15 17:32 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (43.26 KB, patch)
2018-06-15 17:43 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (43.35 KB, patch)
2018-06-15 17:52 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Track Vector and HashTable capacity (53.62 KB, patch)
2018-06-16 12:09 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (53.65 KB, patch)
2018-06-16 13:03 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (53.79 KB, patch)
2018-06-16 13:55 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (53.84 KB, patch)
2018-06-16 16:23 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (53.32 KB, patch)
2018-10-15 11:28 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (56.82 KB, patch)
2020-01-31 14:44 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (55.24 KB, patch)
2021-04-03 10:18 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (50.21 KB, patch)
2023-02-01 21:54 PST, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (50.90 KB, patch)
2023-02-03 21:04 PST, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-06-15 14:58:00 PDT
<rdar://problem/41173203>
Simon Fraser (smfr)
Comment 2 2018-06-15 17:32:48 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 3 2018-06-15 17:34:00 PDT Comment hidden (obsolete)
Simon Fraser (smfr)
Comment 4 2018-06-15 17:43:04 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 5 2018-06-15 17:44:53 PDT Comment hidden (obsolete)
Simon Fraser (smfr)
Comment 6 2018-06-15 17:52:43 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 7 2018-06-15 17:54:17 PDT Comment hidden (obsolete)
Simon Fraser (smfr)
Comment 8 2018-06-16 12:09:05 PDT
Created attachment 342886 [details] Track Vector and HashTable capacity
EWS Watchlist
Comment 9 2018-06-16 12:11:58 PDT
Attachment 342886 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:40: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:64: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:65: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Simon Fraser (smfr)
Comment 10 2018-06-16 13:03:09 PDT
Created attachment 342887 [details] Patch
EWS Watchlist
Comment 11 2018-06-16 13:04:48 PDT
Attachment 342887 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:40: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:64: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:65: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Simon Fraser (smfr)
Comment 12 2018-06-16 13:35:50 PDT
This builds for me on clang 900.0.39. I wonder what version the bots are using.
Simon Fraser (smfr)
Comment 13 2018-06-16 13:54:07 PDT
Also builds for me locally if I pass -std=gnu++1y which the bots are doing. My local build uses -std=gnu++14 by default. Why are we not using the same arguments everywhere?
Simon Fraser (smfr)
Comment 14 2018-06-16 13:55:43 PDT
Created attachment 342889 [details] Patch
EWS Watchlist
Comment 15 2018-06-16 13:57:51 PDT
Attachment 342889 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:40: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:64: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:65: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Simon Fraser (smfr)
Comment 16 2018-06-16 16:23:33 PDT
Created attachment 342893 [details] Patch
EWS Watchlist
Comment 17 2018-06-16 16:25:21 PDT
Attachment 342893 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ContainerCapacityTracker.cpp:114: Use 'WTFMove()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:40: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:64: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/ContainerCapacityTracker.h:65: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 5 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alexey Proskuryakov
Comment 18 2018-06-17 01:41:58 PDT
Mac bots where this fails have Xcode 8.3.3, and pass no special options (it's just build-webkit through webkit-patch). 8.3.3 is the newest version that exists for macOS Sierra. > Also builds for me locally if I pass -std=gnu++1y which the bots are doing. My local build uses -std=gnu++14 by default. Why are we not using the same arguments everywhere? IIRC Yusuke found that different option names are needed for Xcode 8.3 and for Xcode 9.
Darin Adler
Comment 19 2018-06-17 11:14:41 PDT
Comment on attachment 342893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342893&action=review > Source/WTF/wtf/Vector.h:952 > +#if TRACK_VECTOR_CAPACITY > + vectorTracker().recordSizeAndCapacity(m_trackedID, sizeInBytes(), capacityInBytes()); > +#endif This should be a single member function, with an empty inline definition for when TRACK_VECTOR_CAPACITY is off. So we don’t need to paste this three line idiom in all the functions.
Simon Fraser (smfr)
Comment 20 2018-06-18 13:40:21 PDT
Are EWS using the most recent version of the tools that we need to support, or can we update them?
Alexey Proskuryakov
Comment 21 2018-06-18 13:51:28 PDT
That's what needs to be supported.
Rob Buis
Comment 22 2018-10-14 13:59:54 PDT
The patch does not apply for me 100% (I use git svn), just a tiny conflict in StackShot.h. How about doing this to filter out the 0 byte waste announcements for Vector?: diff --git a/Source/WTF/wtf/Vector.cpp b/Source/WTF/wtf/Vector.cpp index 9abb6df014f..547e92afe5e 100644 --- a/Source/WTF/wtf/Vector.cpp +++ b/Source/WTF/wtf/Vector.cpp @@ -46,7 +46,7 @@ static void dumpWastedVectorCapacity() // Make a vector of raw pointers so we can sort them. std::vector<AllocationCallSiteData*> callSitePtrs; for (const auto& siteData : callSiteData) { - if (!siteData.capacity) + if (!siteData.capacity || (siteData.capacity == siteData.usedSize)) continue; callSitePtrs.push_back(const_cast<AllocationCallSiteData*>(&siteData)); }
Simon Fraser (smfr)
Comment 23 2018-10-14 22:18:35 PDT
(In reply to Rob Buis from comment #22) > The patch does not apply for me 100% (I use git svn), just a tiny conflict > in StackShot.h. > How about doing this to filter out the 0 byte waste announcements for > Vector?: > diff --git a/Source/WTF/wtf/Vector.cpp b/Source/WTF/wtf/Vector.cpp > index 9abb6df014f..547e92afe5e 100644 > --- a/Source/WTF/wtf/Vector.cpp > +++ b/Source/WTF/wtf/Vector.cpp > @@ -46,7 +46,7 @@ static void dumpWastedVectorCapacity() > // Make a vector of raw pointers so we can sort them. > std::vector<AllocationCallSiteData*> callSitePtrs; > for (const auto& siteData : callSiteData) { > - if (!siteData.capacity) > + if (!siteData.capacity || (siteData.capacity == siteData.usedSize)) > continue; > > callSitePtrs.push_back(const_cast<AllocationCallSiteData*>(&siteData)); > } Sure. Feel free to attach a new patch.
Rob Buis
Comment 24 2018-10-15 11:28:05 PDT
Created attachment 352346 [details] Patch
Frédéric Wang (:fredw)
Comment 25 2018-11-05 05:25:48 PST
Comment on attachment 352346 [details] Patch It seems this patch does not build on non-Cocoa platforms?
Yusuke Suzuki
Comment 26 2020-01-31 14:44:54 PST
Created attachment 389418 [details] Patch
Simon Fraser (smfr)
Comment 27 2021-04-03 10:18:44 PDT
Created attachment 425098 [details] Patch
Simon Fraser (smfr)
Comment 28 2023-02-01 21:54:29 PST
Created attachment 464809 [details] Patch
Simon Fraser (smfr)
Comment 29 2023-02-02 16:16:17 PST
*** Bug 186699 has been marked as a duplicate of this bug. ***
Simon Fraser (smfr)
Comment 30 2023-02-03 21:04:25 PST
Created attachment 464831 [details] Patch
Note You need to log in before you can comment on or make changes to this bug.