Make it possible to track call sites that waste Vector and HashMap capacity

[Simon Fraser (smfr)]

We should have tooling that we can enable that allows us to find call sites that waste vector capacity.

[Radar WebKit Bug Importer]

<rdar://problem/41173203>

[Simon Fraser (smfr)]

Created attachment 342860 [details]
Patch

[EWS Watchlist]

Attachment 342860 [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 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Created attachment 342861 [details]
Patch

[EWS Watchlist]

Attachment 342861 [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 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Created attachment 342862 [details]
Patch

[EWS Watchlist]

Attachment 342862 [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 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Created attachment 342886 [details]
Track Vector and HashTable capacity

[EWS Watchlist]

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)]

Created attachment 342887 [details]
Patch

[EWS Watchlist]

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)]

This builds for me on clang 900.0.39. I wonder what version the bots are using.

[Simon Fraser (smfr)]

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)]

Created attachment 342889 [details]
Patch

[EWS Watchlist]

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)]

Created attachment 342893 [details]
Patch

[EWS Watchlist]

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]

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 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)]

Are EWS using the most recent version of the tools that we need to support, or can we update them?

[Alexey Proskuryakov]

That's what needs to be supported.

[Rob Buis]

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)]

(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]

Created attachment 352346 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 352346 [details]
Patch

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

[Yusuke Suzuki]

Created attachment 389418 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 425098 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 464809 [details]
Patch

[Simon Fraser (smfr)]

*** Bug 186699 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

Created attachment 464831 [details]
Patch