Add an SVM-based classification method for resource load statistics.
rdar://problem/30352793
Created attachment 301570 [details] Patch
Comment on attachment 301570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301570&action=review > Source/WebKit2/ChangeLog:53 > + * libsvm_model: Added. This should go in a subdirectory. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:71 > +#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) > +#if USE(CF) Inner conditions probably not necessary. All Cocoa platforms use CF. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:32 > +#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) This is used a lot. Let's give it a name. #if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) #define HAVE_SVM 1 #endif
Comment on attachment 301570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301570&action=review Overall, I think you need to make a platform abstraction around the CorePrediction stuff, and use the abstraction instead of svm_model/etc. directly. > Source/WebKit2/ChangeLog:9 > + This patch adds an SVM-based classifier to the WebResourceLoadStatisticsStore. What is SVM? Support Vector Machine? Could use a definition since it's not the most well known acronym. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:37 > +#if USE(CF) > +#include <CoreFoundation/CoreFoundation.h> > +#endif Please don't include CoreFoundation like this in a cross platform class. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:53 > +#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) > +static bool useSVM = true; > +#endif It seems odd to have this be a global? Usually setting like this are in WebPreferences. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:81 > + CFBundleRef webKitBundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebKit")); > + CFURLRef resourceUrl = CFBundleCopyResourcesDirectoryURL(webKitBundle); > + resourceUrl = CFURLCreateCopyAppendingPathComponent(nullptr, resourceUrl, CFSTR("libsvm_model"), false); > + CFErrorRef error = nullptr; > + resourceUrl = CFURLCreateFilePathURL(nullptr, resourceUrl, &error); > + CFURLPathStyle pathStyle = kCFURLPOSIXPathStyle; > + if (!error && resourceUrl) { > + CFStringRef resourceUrlString = CFURLCopyFileSystemPath(resourceUrl, pathStyle); > + m_svmModelStoragePath = String(resourceUrlString); > + } Can this be moved into a platform abstraction? Having CF code like this in a cross platform class is not something we like to do.
Created attachment 301858 [details] Patch
Thanks for looking at this, Alex! (In reply to comment #3) > Comment on attachment 301570 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301570&action=review > > > Source/WebKit2/ChangeLog:53 > > + * libsvm_model: Added. > > This should go in a subdirectory. For sure. Fixed. > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:71 > > +#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) > > +#if USE(CF) > > Inner conditions probably not necessary. All Cocoa platforms use CF. Fixed. > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:32 > > +#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) > > This is used a lot. Let's give it a name. > #if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= > 101200) > #define HAVE_SVM 1 > #endif Fixed.
Thanks for your review, Sam! (In reply to comment #4) > Comment on attachment 301570 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301570&action=review > > Overall, I think you need to make a platform abstraction around the > CorePrediction stuff, and use the abstraction instead of svm_model/etc. > directly. I agree. Fixed as a platform abstraction. > > Source/WebKit2/ChangeLog:9 > > + This patch adds an SVM-based classifier to the WebResourceLoadStatisticsStore. > > What is SVM? Support Vector Machine? Could use a definition since it's not > the most well known acronym. Consistently renamed CorePrediction. > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:37 > > +#if USE(CF) > > +#include <CoreFoundation/CoreFoundation.h> > > +#endif > > Please don't include CoreFoundation like this in a cross platform class. Fixed. > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:53 > > +#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) > > +static bool useSVM = true; > > +#endif > > It seems odd to have this be a global? Usually setting like this are in > WebPreferences. It is no longer needed as a global and is now a member variable of the Cocoa classifier class. > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:81 > > + CFBundleRef webKitBundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebKit")); > > + CFURLRef resourceUrl = CFBundleCopyResourcesDirectoryURL(webKitBundle); > > + resourceUrl = CFURLCreateCopyAppendingPathComponent(nullptr, resourceUrl, CFSTR("libsvm_model"), false); > > + CFErrorRef error = nullptr; > > + resourceUrl = CFURLCreateFilePathURL(nullptr, resourceUrl, &error); > > + CFURLPathStyle pathStyle = kCFURLPOSIXPathStyle; > > + if (!error && resourceUrl) { > > + CFStringRef resourceUrlString = CFURLCopyFileSystemPath(resourceUrl, pathStyle); > > + m_svmModelStoragePath = String(resourceUrlString); > > + } > > Can this be moved into a platform abstraction? Having CF code like this in a > cross platform class is not something we like to do. Yes, this is a great suggestion. Fixed.
Alexey has given me some pointer on how to get EWS builds for iOS green. I will look into it but would appreciate a review of the code as is. I might even enable on iOS in a follow-up patch.
Created attachment 301862 [details] Patch
Fixed my #if:s into #ifdef:s for HAVE_CORE_PREDICTION.
Comment on attachment 301862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301862&action=review > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.h:32 > +// FIXME: Add PLATFORM(IOS) as a supported platform once we have figured out > +// how to support private frameworks on EWS. > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) > +#define HAVE_CORE_PREDICTION 1 > +#endif This should move to WTF's Platform.h and users of it should use the HAVE(CORE_PREDICTION) form.
Created attachment 301879 [details] Patch
(In reply to comment #11) > Comment on attachment 301862 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301862&action=review > > > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.h:32 > > +// FIXME: Add PLATFORM(IOS) as a supported platform once we have figured out > > +// how to support private frameworks on EWS. > > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) > > +#define HAVE_CORE_PREDICTION 1 > > +#endif > > This should move to WTF's Platform.h and users of it should use the > HAVE(CORE_PREDICTION) form. Got it. Fixed.
Created attachment 301882 [details] Patch
Created attachment 301887 [details] Patch
Compiler got confused about what it thought was a missing return. Added an attempt to fix the three cmake files by adding the classifier directory and implementation file(s).
Comment on attachment 301887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301887&action=review > Source/WTF/wtf/Platform.h:1211 > +#if PLATFORM(IOS) && USE(APPLE_INTERNAL_SDK) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) This should be PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) > Source/WebKit2/Configurations/WebKit.xcconfig:37 > +FRAMEWORK_AND_LIBRARY_LDFLAGS_BASE_ios = -lobjc -framework AssertionServices -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CorePDF -framework CorePrediction -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework OpenGLES -framework MobileCoreServices -lMobileGestalt $(FRAMEWORK_AND_LIBRARY_LDFLAGS_PLATFORM_$(PLATFORM_NAME)); To make this link, please add a framework stub for iOS in WebKitLibraries/WebKitPrivateFrameworkStubs. > Source/WebKit2/Configurations/WebKit.xcconfig:44 > +FRAMEWORK_AND_LIBRARY_LDFLAGS[sdk=macosx*] = -framework ApplicationServices -framework Carbon -framework Cocoa -framework CoreServices -framework IOKit -framework CoreAudio -framework IOSurface -framework OpenGL $(FRAMEWORK_AND_LIBRARY_LDFLAGS_macosx_$(TARGET_MAC_OS_X_VERSION_MAJOR)); > +FRAMEWORK_AND_LIBRARY_LDFLAGS_macosx_101200 = -framework CorePrediction; > +FRAMEWORK_AND_LIBRARY_LDFLAGS_macosx_101300 = -framework CorePrediction; I think that it would be cleaner to use a different name for the extra, as our normal pattern is to put the whole list in the _$(TARGET_MAC_OS_X_VERSION_MAJOR) variable. Maybe something like EXTRA_FRAMEWORK_AND_LIBRARY_LDFLAGS_101200 would be best. > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.h:32 > +#if HAVE(CORE_PREDICTION) > +#include <CorePrediction/svm.h> > +#endif This should be in a separate CorePredictionSPI.h header. After conditionally importing svm.h, please declare all functions and types that you use (see other SPI headers for example). For the include check, please use #if USE(APPLE_INTERNAL_SDK) && HAVE(CORE_PREDICTION).
Created attachment 302014 [details] Patch
(In reply to comment #17) > Comment on attachment 301887 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301887&action=review > > > Source/WTF/wtf/Platform.h:1211 > > +#if PLATFORM(IOS) && USE(APPLE_INTERNAL_SDK) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) > > This should be PLATFORM(IOS) || (PLATFORM(MAC) && > __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) Fixed. > > Source/WebKit2/Configurations/WebKit.xcconfig:37 > > +FRAMEWORK_AND_LIBRARY_LDFLAGS_BASE_ios = -lobjc -framework AssertionServices -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CorePDF -framework CorePrediction -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework OpenGLES -framework MobileCoreServices -lMobileGestalt $(FRAMEWORK_AND_LIBRARY_LDFLAGS_PLATFORM_$(PLATFORM_NAME)); > > To make this link, please add a framework stub for iOS in > WebKitLibraries/WebKitPrivateFrameworkStubs. Fixed. Please review since it's the first time I've added a framework stub. > > Source/WebKit2/Configurations/WebKit.xcconfig:44 > > +FRAMEWORK_AND_LIBRARY_LDFLAGS[sdk=macosx*] = -framework ApplicationServices -framework Carbon -framework Cocoa -framework CoreServices -framework IOKit -framework CoreAudio -framework IOSurface -framework OpenGL $(FRAMEWORK_AND_LIBRARY_LDFLAGS_macosx_$(TARGET_MAC_OS_X_VERSION_MAJOR)); > > +FRAMEWORK_AND_LIBRARY_LDFLAGS_macosx_101200 = -framework CorePrediction; > > +FRAMEWORK_AND_LIBRARY_LDFLAGS_macosx_101300 = -framework CorePrediction; > > I think that it would be cleaner to use a different name for the extra, as > our normal pattern is to put the whole list in the > _$(TARGET_MAC_OS_X_VERSION_MAJOR) variable. > > Maybe something like EXTRA_FRAMEWORK_AND_LIBRARY_LDFLAGS_101200 would be > best. Fixed. > > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.h:32 > > +#if HAVE(CORE_PREDICTION) > > +#include <CorePrediction/svm.h> > > +#endif > > This should be in a separate CorePredictionSPI.h header. After conditionally > importing svm.h, please declare all functions and types that you use (see > other SPI headers for example). > > For the include check, please use #if USE(APPLE_INTERNAL_SDK) && > HAVE(CORE_PREDICTION). Fixed. Thanks, Alexey!
Created attachment 302018 [details] Patch
Attachment 302018 [details] did not pass style-queue: ERROR: Source/WebKit2/Platform/classifier/cocoa/CorePredictionSPI.h:39: svm_predict_values is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #21) > Attachment 302018 [details] did not pass style-queue: > > > ERROR: Source/WebKit2/Platform/classifier/cocoa/CorePredictionSPI.h:39: > svm_predict_values is incorrectly named. Don't use underscores in your > identifier names. [readability/naming/underscores] [4] > Total errors found: 1 in 52 files This is a deliberate violation, declaring symbols from CorePrediction in case we can't include the real header file.
Created attachment 302022 [details] Patch
(In reply to comment #23) > Created attachment 302022 [details] > Patch svm_load_model() was missing from the re-declaration in the SPI file.
Attachment 302022 [details] did not pass style-queue: ERROR: Source/WebKit2/Platform/classifier/cocoa/CorePredictionSPI.h:39: svm_predict_values is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
View in context: https://bugs.webkit.org/attachment.cgi?id=302018&action=review > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:76 > + CFURLRef resourceUrl = CFBundleCopyResourcesDirectoryURL(webKitBundle); You should be using RetainPtr<> for this stuff. The "Copy/Create" rule of Objective C says that CFBundleCopyResources... is returning a CFURLRef with a +1 reference count. This is getting leaked when you pass it to CFURLCreateCopyAppendingPathComponent, which creates a brand new CFURL with a +1 count. Use: RetainPtr<CFURLRef> resourceURL = adoptCF(CFBundleCopyResourcesDirectoryURL(webKitBundle)); > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:77 > + resourceUrl = CFURLCreateCopyAppendingPathComponent(nullptr, resourceUrl, CFSTR("corePrediction_model"), false); Same thing: resourceURL = adoptCF(CFURLCreateCopyAppendingPathComponent(nullptr, resourceUrl.get(), CFSTR("corePrediction_model"), false); > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:79 > + resourceUrl = CFURLCreateFilePathURL(nullptr, resourceUrl, &error); and again: resourceURL = adoptCF(CFURLCreateFilePathURL(nullptr, resourceURL.get(), &error); > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:85 > + CFStringRef resourceUrlString = CFURLCopyFileSystemPath(resourceUrl, pathStyle); RetainPtr<CFStringRef> resourceURLString = adoptCF(CFURLCopyFileSystemPath(resourceURL.get(), kCFURLPOSIXPathStyle); > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:86 > + return String(resourceUrlString); return String(resourceURLString.get());
Created attachment 302172 [details] Patch
(In reply to comment #26) > View in context: > https://bugs.webkit.org/attachment.cgi?id=302018&action=review > > > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:76 > > + CFURLRef resourceUrl = CFBundleCopyResourcesDirectoryURL(webKitBundle); > > You should be using RetainPtr<> for this stuff. > > The "Copy/Create" rule of Objective C says that CFBundleCopyResources... is > returning a CFURLRef with a +1 reference count. This is getting leaked when > you pass it to CFURLCreateCopyAppendingPathComponent, which creates a brand > new CFURL with a +1 count. > > Use: > RetainPtr<CFURLRef> resourceURL = > adoptCF(CFBundleCopyResourcesDirectoryURL(webKitBundle)); > > > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:77 > > + resourceUrl = CFURLCreateCopyAppendingPathComponent(nullptr, resourceUrl, CFSTR("corePrediction_model"), false); > > Same thing: > > resourceURL = adoptCF(CFURLCreateCopyAppendingPathComponent(nullptr, > resourceUrl.get(), CFSTR("corePrediction_model"), false); > > > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:79 > > + resourceUrl = CFURLCreateFilePathURL(nullptr, resourceUrl, &error); > > and again: > > resourceURL = adoptCF(CFURLCreateFilePathURL(nullptr, resourceURL.get(), > &error); > > > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:85 > > + CFStringRef resourceUrlString = CFURLCopyFileSystemPath(resourceUrl, pathStyle); > > RetainPtr<CFStringRef> resourceURLString = > adoptCF(CFURLCopyFileSystemPath(resourceURL.get(), kCFURLPOSIXPathStyle); > > > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:86 > > + return String(resourceUrlString); > > return String(resourceURLString.get()); Thanks, Brent! I fixed all of these.
Attachment 302172 [details] did not pass style-queue: ERROR: Source/WebKit2/Platform/classifier/cocoa/CorePredictionSPI.h:45: svm_predict_values is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 302172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302172&action=review > Source/WebKit2/Platform/classifier/ResourceLoadStatisticsClassifierBase.cpp:35 > +static const auto featureVectorLengthThreshold = 3; Moving this might make sense, but it might also be useful to have one collection of the values we can tune them easier. > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:55 > + svm_node* exampleVector = new svm_node[nonZeroFeatures.size() + 1]; Can we use a std::unique_ptr<svm_node[]> instead of new and delete? I know right now it looks obvious that there is one code path, but it might become more complex, and there's no reason to use new and delete. We might even want Vector<svm_node> instead to get the bounds checking that comes with Vector. > LayoutTests/http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-subresource-unique-redirects-to.html:33 > + testRunner.statisticsResetToConsistentState(); We might want to call this from Internals::resetToConsistentState so we won't get flaky tests in tests run after this if this tests fails to call this for some reason.
(In reply to comment #30) > Comment on attachment 302172 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302172&action=review > > > Source/WebKit2/Platform/classifier/ResourceLoadStatisticsClassifierBase.cpp:35 > > +static const auto featureVectorLengthThreshold = 3; > > Moving this might make sense, but it might also be useful to have one > collection of the values we can tune them easier. We will revisit depending on whether we want to expose these settings in API or have them in a file outside the bundle. > > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:55 > > + svm_node* exampleVector = new svm_node[nonZeroFeatures.size() + 1]; > > Can we use a std::unique_ptr<svm_node[]> instead of new and delete? I know > right now it looks obvious that there is one code path, but it might become > more complex, and there's no reason to use new and delete. > We might even want Vector<svm_node> instead to get the bounds checking that > comes with Vector. That's a good suggestion. Saving for later since we need to land this. > > LayoutTests/http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-subresource-unique-redirects-to.html:33 > > + testRunner.statisticsResetToConsistentState(); > > We might want to call this from Internals::resetToConsistentState so we > won't get flaky tests in tests run after this if this tests fails to call > this for some reason. Yes, I did look into that but since Internals operates on WebCore and my TestRunner functions go through WebKit2 I wasn't sure where to have the single point where we reset the statistics. I filed https://bugs.webkit.org/show_bug.cgi?id=168619 for this task. Thanks for the reviews, Alex, Sam, Alexey, and Brent!
Comment on attachment 302172 [details] Patch Clearing flags on attachment: 302172 Committed r212685: <http://trac.webkit.org/changeset/212685>
All reviewed patches have been landed. Closing bug.
This change appears to have broken the Sierra 32-bit build: Undefined symbols for architecture i386: "_svm_load_model", referenced from: WebKit::ResourceLoadStatisticsClassifier::shouldUseCorePrediction() in ResourceLoadStatisticsClassifierCocoa.o "_svm_predict_values", referenced from: WebKit::ResourceLoadStatisticsClassifier::classify(unsigned int, unsigned int, unsigned int) in ResourceLoadStatisticsClassifierCocoa.o ld: symbol(s) not found for architecture i386 https://build.webkit.org/builders/Apple%20Sierra%20Release%20%2832-bit%20Build%29/builds/3867
Reverted r212685 for reason: This change broke the 32-bit Sierra build. Committed r212691: <http://trac.webkit.org/changeset/212691>
Created attachment 302281 [details] Patch
Attachment 302281 [details] did not pass style-queue: ERROR: Source/WebKit2/Platform/classifier/cocoa/CorePredictionSPI.h:45: svm_predict_values is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 302281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302281&action=review r=me for the new HAVE(CORE_PREDICTION) macro. > Source/WebKit2/Configurations/BaseTarget.xcconfig:35 > +FRAMEWORK_SEARCH_PATHS_base = "$(UMBRELLA_FRAMEWORKS_DIR)" /System/Library/PrivateFrameworks $(FRAMEWORK_SEARCH_PATHS); Is this correct? I thought this was what the "spi" folders were used for? > Source/WebKit2/Configurations/WebKit.xcconfig:44 > +EXTRA_FRAMEWORK_AND_LIBRARY_LDFLAGS_101300 = -framework CorePrediction; I'm worried that CorePrediction will fail to link here on the 32-bit Sierra build. This might need to be soft-linked.
Comment on attachment 302281 [details] Patch Thanks for having a look again, Brent. Flagging for commit. We can change the frameworks path in a follow-up if needed.
Comment on attachment 302281 [details] Patch Clearing flags on attachment: 302281 Committed r212757: <http://trac.webkit.org/changeset/212757>
Committed r212800: <http://trac.webkit.org/changeset/212800>
(In reply to comment #42) > Committed r212800: <http://trac.webkit.org/changeset/212800> This is a temporary skip of the test cases while we investigate bot test crashes in https://bugs.webkit.org/show_bug.cgi?id=168703.
Reopening to attach new patch.
Created attachment 302473 [details] Patch
Attachment 302473 [details] did not pass style-queue: ERROR: Source/WebKit2/Platform/classifier/cocoa/CorePredictionSPI.h:45: svm_predict_values is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 56 files If any of these errors are false positives, please file a bug against check-webkit-style.
The previous patch was rolled out in https://trac.webkit.org/changeset/212841.
The new things that need a re-review. 1. WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp a) Soft linking: SOFT_LINK_PRIVATE_FRAMEWORK_OPTIONAL(CorePrediction) SOFT_LINK(CorePrediction, svm_predict_values, double, (const struct svm_model *model, const struct svm_node *x, double* dec_values), (model, x, dec_values)) SOFT_LINK(CorePrediction, svm_load_model, svm_model*, (const char *model_file_name), (model_file_name)) b) New function with static variable: struct svm_model* ResourceLoadStatisticsClassifier::singletonPredictionModel() c) Added logging. 2. WebKit2/Platform/classifier/ResourceLoadStatisticsClassifierBase.cpp a) Added logging. 3. WebKit2/Platform/Logging.h a) Added a channel. 4. Testcases a) Now call internals.setResourceLoadStatisticsEnabled(false) before testRunner.notifyDone().
It would be great if David could double-check that we did the SOFT_LINK stuff properly.
Comment on attachment 302473 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302473&action=review Looks good to me. Alex or another anointed WK2 reviewer needs to do final signoff. > Source/WebKit2/Platform/classifier/ResourceLoadStatisticsClassifierBase.h:41 > + bool classifyWithVectorThreshold(const unsigned, const unsigned, const unsigned); nit: It's not really necessary to pass unsigned as const, unless we want to be sure 'classifyWithVectorThreshold' does not modify its input values for some reason. > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:61 > + svm_node* exampleVector = new svm_node[nonZeroFeatures.size() + 1]; Rather than new/delete, we should use a Vector<svm_node*>: Vector<svm_node*> exampleVector(nonZeroFeatures.size() + 1); > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:65 > + } Add a newline, please > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:72 > + classification = svm_predict_values(singletonPredictionModel(), exampleVector, &score); int classification = svm_predict_values(singletonPredictionModel(), exampleVector.data(), &score); > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:73 > + delete[] exampleVector; Then this could go away... > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:1243 > + rawValues.resize(values.size()); I think these can just be: Vector<WKStringRef> rawKeys(keys.size()); Vector<WKTypeRef> rawValues(values.size()); This should create two vectors, sized properly with default values (hopefully nullptr). > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:1269 > + rawValues.resize(values.size()); Ditto above -- just declare vectors of the right size, rather than do it in two steps.
I can confirm that the release-mode crashes I was getting with the older patch are resolved with this new version.
Comment on attachment 302473 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302473&action=review > Source/WebKit2/Platform/classifier/cocoa/CorePredictionSPI.h:47 > +struct svm_model *svm_load_model(const char*); I think we need soft linking headers instead of this. See CoreVideoSoftLink.h and CoreVideoSoftLink.cpp as an example.
Created attachment 302626 [details] Patch
Created attachment 302640 [details] Patch
Created attachment 302641 [details] Patch
Comment on attachment 302641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302641&action=review > Source/WebKit2/Platform/classifier/cocoa/CorePredictionSoftLink.h:28 > +#if USE(APPLE_INTERNAL_SDK) && HAVE(CORE_PREDICTION) We should make this buildable for open-source contributors who don't have the internal sdk. If we declare what we use and soft link, it should work. Then we can have the open source bots having the same behavior as the internal bots. > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:89 > +bool ResourceLoadStatisticsClassifier::shouldUseCorePredictionAndHaveModelLoaded() Would canUseCorePrediction be a better name? You can't use it if you don't have a model loaded. > WebKitLibraries/ChangeLog:11 > + * WebKitPrivateFrameworkStubs/iOS/10/CorePrediction.framework/CorePrediction.tbd: Added. Is this still needed when we soft link?
Comment on attachment 302641 [details] Patch I'm going to clean this up a bit.
(In reply to comment #56) > Comment on attachment 302641 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302641&action=review > > > Source/WebKit2/Platform/classifier/cocoa/CorePredictionSoftLink.h:28 > > +#if USE(APPLE_INTERNAL_SDK) && HAVE(CORE_PREDICTION) > > We should make this buildable for open-source contributors who don't have > the internal sdk. If we declare what we use and soft link, it should work. > Then we can have the open source bots having the same behavior as the > internal bots. To the best of my knowledge this is buildable and usable by open source platforms without internal SDK. It's designed that way. > > Source/WebKit2/Platform/classifier/cocoa/ResourceLoadStatisticsClassifierCocoa.cpp:89 > > +bool ResourceLoadStatisticsClassifier::shouldUseCorePredictionAndHaveModelLoaded() > > Would canUseCorePrediction be a better name? You can't use it if you don't > have a model loaded. Maybe. I guess I had thoughts on it being configurable but maybe that's not really what we want. > > WebKitLibraries/ChangeLog:11 > > + * WebKitPrivateFrameworkStubs/iOS/10/CorePrediction.framework/CorePrediction.tbd: Added. > > Is this still needed when we soft link? Maybe not.
Created attachment 302655 [details] Patch
Comment on attachment 302655 [details] Patch I cleaned it up a bit. I made it build for non-Apple-internal builds, I got rid of ResourceLoadStatisticsClassifierBase in favor of just ResourceLoadStatisticsClassifier and ResourceLoadStatisticsClassifierCocoa, I made the soft linking work without including the CorePrediction headers directly, and I removed unnecessary Vector allocation when generating the feature vector. This passes all the new tests on my computer. Feel free to land. r=me
(In reply to comment #60) > Comment on attachment 302655 [details] > Patch > > I cleaned it up a bit. I made it build for non-Apple-internal builds, I got > rid of ResourceLoadStatisticsClassifierBase in favor of just > ResourceLoadStatisticsClassifier and ResourceLoadStatisticsClassifierCocoa, > I made the soft linking work without including the CorePrediction headers > directly, and I removed unnecessary Vector allocation when generating the > feature vector. This passes all the new tests on my computer. Feel free to > land. r=me Thanks! The split into a Base class was because that seemed to be the way we do it and other platforms may want to use open source libsvm. But that's future work. I'll land it and check tomorrow on a debug build that we're actually executing on CorePrediction on internal SDKs. The tests pass either way (intended).
Comment on attachment 302655 [details] Patch Clearing flags on attachment: 302655 Committed r212949: <http://trac.webkit.org/changeset/212949>