Summary: | Make it possible to track call sites that waste Vector and HashMap capacity | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||||||||||||
Component: | Tools / Tests | Assignee: | 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
Simon Fraser (smfr)
2018-06-15 14:57:37 PDT
Created attachment 342860 [details]
Patch
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.
Created attachment 342861 [details]
Patch
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.
Created attachment 342862 [details]
Patch
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.
Created attachment 342886 [details]
Track Vector and HashTable capacity
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.
Created attachment 342887 [details]
Patch
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.
This builds for me on clang 900.0.39. I wonder what version the bots are using. 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? Created attachment 342889 [details]
Patch
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.
Created attachment 342893 [details]
Patch
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.
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 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. Are EWS using the most recent version of the tools that we need to support, or can we update them? That's what needs to be supported. 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)); } (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. Created attachment 352346 [details]
Patch
Comment on attachment 352346 [details]
Patch
It seems this patch does not build on non-Cocoa platforms?
Created attachment 389418 [details]
Patch
Created attachment 425098 [details]
Patch
Created attachment 464809 [details]
Patch
*** Bug 186699 has been marked as a duplicate of this bug. *** Created attachment 464831 [details]
Patch
|