Bug 168347 - Resource Load Statistics: Add alternate classification method
Summary: Resource Load Statistics: Add alternate classification method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-14 17:55 PST by John Wilander
Modified: 2017-02-24 01:21 PST (History)
13 users (show)

See Also:


Attachments
Patch (83.82 KB, patch)
2017-02-14 18:20 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (110.74 KB, patch)
2017-02-16 16:54 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (110.75 KB, patch)
2017-02-16 17:05 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (111.53 KB, patch)
2017-02-16 21:24 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (110.28 KB, patch)
2017-02-16 22:21 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (113.28 KB, patch)
2017-02-16 23:22 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (120.71 KB, patch)
2017-02-17 16:56 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (119.81 KB, patch)
2017-02-17 17:08 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (119.86 KB, patch)
2017-02-17 17:31 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (120.05 KB, patch)
2017-02-20 14:46 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (120.05 KB, patch)
2017-02-21 10:36 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (122.87 KB, patch)
2017-02-22 17:57 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (121.86 KB, patch)
2017-02-23 18:33 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (65.50 KB, patch)
2017-02-23 21:38 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (122.05 KB, patch)
2017-02-23 21:45 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (116.18 KB, patch)
2017-02-24 00:35 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2017-02-14 17:55:45 PST
Add an SVM-based classification method for resource load statistics.
Comment 1 John Wilander 2017-02-14 17:56:17 PST
rdar://problem/30352793
Comment 2 John Wilander 2017-02-14 18:20:42 PST
Created attachment 301570 [details]
Patch
Comment 3 Alex Christensen 2017-02-15 00:15:17 PST
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 4 Sam Weinig 2017-02-15 09:10:34 PST
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.
Comment 5 John Wilander 2017-02-16 16:54:28 PST
Created attachment 301858 [details]
Patch
Comment 6 John Wilander 2017-02-16 16:55:33 PST
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.
Comment 7 John Wilander 2017-02-16 16:57:47 PST
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.
Comment 8 John Wilander 2017-02-16 17:01:24 PST
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.
Comment 9 John Wilander 2017-02-16 17:05:43 PST
Created attachment 301862 [details]
Patch
Comment 10 John Wilander 2017-02-16 17:06:40 PST
Fixed my #if:s into #ifdef:s for HAVE_CORE_PREDICTION.
Comment 11 Sam Weinig 2017-02-16 18:05:19 PST
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.
Comment 12 John Wilander 2017-02-16 21:24:54 PST
Created attachment 301879 [details]
Patch
Comment 13 John Wilander 2017-02-16 21:25:35 PST
(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.
Comment 14 John Wilander 2017-02-16 22:21:46 PST
Created attachment 301882 [details]
Patch
Comment 15 John Wilander 2017-02-16 23:22:56 PST
Created attachment 301887 [details]
Patch
Comment 16 John Wilander 2017-02-16 23:24:35 PST
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 17 Alexey Proskuryakov 2017-02-17 15:03:47 PST
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).
Comment 18 John Wilander 2017-02-17 16:56:20 PST
Created attachment 302014 [details]
Patch
Comment 19 John Wilander 2017-02-17 16:58:40 PST
(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!
Comment 20 John Wilander 2017-02-17 17:08:25 PST
Created attachment 302018 [details]
Patch
Comment 21 WebKit Commit Bot 2017-02-17 17:10:58 PST
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.
Comment 22 John Wilander 2017-02-17 17:29:28 PST
(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.
Comment 23 John Wilander 2017-02-17 17:31:57 PST
Created attachment 302022 [details]
Patch
Comment 24 John Wilander 2017-02-17 17:33:06 PST
(In reply to comment #23)
> Created attachment 302022 [details]
> Patch

svm_load_model() was missing from the re-declaration in the SPI file.
Comment 25 WebKit Commit Bot 2017-02-17 17:35:28 PST
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.
Comment 26 Brent Fulgham 2017-02-17 17:55:12 PST
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());
Comment 27 John Wilander 2017-02-20 14:46:18 PST
Created attachment 302172 [details]
Patch
Comment 28 John Wilander 2017-02-20 14:47:49 PST
(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.
Comment 29 WebKit Commit Bot 2017-02-20 14:48:29 PST
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 30 Alex Christensen 2017-02-20 15:56:44 PST
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.
Comment 31 John Wilander 2017-02-20 16:09:39 PST
(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 32 WebKit Commit Bot 2017-02-20 16:36:23 PST
Comment on attachment 302172 [details]
Patch

Clearing flags on attachment: 302172

Committed r212685: <http://trac.webkit.org/changeset/212685>
Comment 33 WebKit Commit Bot 2017-02-20 16:36:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 34 Ryan Haddad 2017-02-20 17:39:33 PST
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
Comment 35 Ryan Haddad 2017-02-20 17:49:59 PST
Reverted r212685 for reason:

This change broke the 32-bit Sierra build.

Committed r212691: <http://trac.webkit.org/changeset/212691>
Comment 36 John Wilander 2017-02-21 10:36:32 PST
Created attachment 302281 [details]
Patch
Comment 37 WebKit Commit Bot 2017-02-21 10:43:41 PST
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 38 Brent Fulgham 2017-02-21 10:48:14 PST
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 39 John Wilander 2017-02-21 14:15:06 PST
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 40 WebKit Commit Bot 2017-02-21 14:42:16 PST
Comment on attachment 302281 [details]
Patch

Clearing flags on attachment: 302281

Committed r212757: <http://trac.webkit.org/changeset/212757>
Comment 41 WebKit Commit Bot 2017-02-21 14:42:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 John Wilander 2017-02-21 20:30:07 PST
Committed r212800: <http://trac.webkit.org/changeset/212800>
Comment 43 John Wilander 2017-02-21 20:31:31 PST
(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.
Comment 44 John Wilander 2017-02-22 17:57:03 PST
Reopening to attach new patch.
Comment 45 John Wilander 2017-02-22 17:57:06 PST
Created attachment 302473 [details]
Patch
Comment 46 WebKit Commit Bot 2017-02-22 17:59:03 PST
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.
Comment 47 John Wilander 2017-02-22 17:59:30 PST
The previous patch was rolled out in https://trac.webkit.org/changeset/212841.
Comment 48 John Wilander 2017-02-22 19:04:20 PST
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().
Comment 49 Brent Fulgham 2017-02-22 20:18:31 PST
It would be great if David could double-check that we did the SOFT_LINK stuff properly.
Comment 50 Brent Fulgham 2017-02-22 20:35:15 PST
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.
Comment 51 Brent Fulgham 2017-02-22 22:14:59 PST
I can confirm that the release-mode crashes I was getting with the older patch are resolved with this new version.
Comment 52 Alex Christensen 2017-02-23 13:54:41 PST
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.
Comment 53 John Wilander 2017-02-23 18:33:40 PST
Created attachment 302626 [details]
Patch
Comment 54 John Wilander 2017-02-23 21:38:29 PST
Created attachment 302640 [details]
Patch
Comment 55 John Wilander 2017-02-23 21:45:16 PST
Created attachment 302641 [details]
Patch
Comment 56 Alex Christensen 2017-02-23 23:23:18 PST
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 57 Alex Christensen 2017-02-23 23:51:39 PST
Comment on attachment 302641 [details]
Patch

I'm going to clean this up a bit.
Comment 58 John Wilander 2017-02-23 23:59:17 PST
(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.
Comment 59 Alex Christensen 2017-02-24 00:35:45 PST
Created attachment 302655 [details]
Patch
Comment 60 Alex Christensen 2017-02-24 00:44:52 PST
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
Comment 61 John Wilander 2017-02-24 00:50:21 PST
(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 62 WebKit Commit Bot 2017-02-24 01:21:42 PST
Comment on attachment 302655 [details]
Patch

Clearing flags on attachment: 302655

Committed r212949: <http://trac.webkit.org/changeset/212949>
Comment 63 WebKit Commit Bot 2017-02-24 01:21:51 PST
All reviewed patches have been landed.  Closing bug.