NEW 186698
Make it possible to track call sites that waste Vector and HashMap capacity 
https://bugs.webkit.org/show_bug.cgi?id=186698
Summary Make it possible to track call sites that waste Vector and HashMap capacity
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.