Bug 186698 - Make it possible to track call sites that waste Vector capacity
Summary: Make it possible to track call sites that waste Vector capacity
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-15 14:57 PDT by Simon Fraser (smfr)
Modified: 2018-11-05 05:25 PST (History)
12 users (show)

See Also:


Attachments
Patch (43.27 KB, patch)
2018-06-15 17:32 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (43.26 KB, patch)
2018-06-15 17:43 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (43.35 KB, patch)
2018-06-15 17:52 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Track Vector and HashTable capacity (53.62 KB, patch)
2018-06-16 12:09 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (53.65 KB, patch)
2018-06-16 13:03 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (53.79 KB, patch)
2018-06-16 13:55 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (53.84 KB, patch)
2018-06-16 16:23 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (53.32 KB, patch)
2018-10-15 11:28 PDT, Rob Buis
rbuis: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 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.
Comment 1 Radar WebKit Bug Importer 2018-06-15 14:58:00 PDT
<rdar://problem/41173203>
Comment 2 Simon Fraser (smfr) 2018-06-15 17:32:48 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2018-06-15 17:34:00 PDT Comment hidden (obsolete)
Comment 4 Simon Fraser (smfr) 2018-06-15 17:43:04 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2018-06-15 17:44:53 PDT Comment hidden (obsolete)
Comment 6 Simon Fraser (smfr) 2018-06-15 17:52:43 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2018-06-15 17:54:17 PDT Comment hidden (obsolete)
Comment 8 Simon Fraser (smfr) 2018-06-16 12:09:05 PDT
Created attachment 342886 [details]
Track Vector and HashTable capacity
Comment 9 Build Bot 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.
Comment 10 Simon Fraser (smfr) 2018-06-16 13:03:09 PDT
Created attachment 342887 [details]
Patch
Comment 11 Build Bot 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.
Comment 12 Simon Fraser (smfr) 2018-06-16 13:35:50 PDT
This builds for me on clang 900.0.39. I wonder what version the bots are using.
Comment 13 Simon Fraser (smfr) 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?
Comment 14 Simon Fraser (smfr) 2018-06-16 13:55:43 PDT
Created attachment 342889 [details]
Patch
Comment 15 Build Bot 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.
Comment 16 Simon Fraser (smfr) 2018-06-16 16:23:33 PDT
Created attachment 342893 [details]
Patch
Comment 17 Build Bot 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.
Comment 18 Alexey Proskuryakov 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.
Comment 19 Darin Adler 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.
Comment 20 Simon Fraser (smfr) 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?
Comment 21 Alexey Proskuryakov 2018-06-18 13:51:28 PDT
That's what needs to be supported.
Comment 22 Rob Buis 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));
     }
Comment 23 Simon Fraser (smfr) 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.
Comment 24 Rob Buis 2018-10-15 11:28:05 PDT
Created attachment 352346 [details]
Patch
Comment 25 Frédéric Wang (:fredw) 2018-11-05 05:25:48 PST
Comment on attachment 352346 [details]
Patch

It seems this patch does not build on non-Cocoa platforms?