Bug 168347 – Resource Load Statistics: Add alternate classification method

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[John Wilander]

Add an SVM-based classification method for resource load statistics.

[John Wilander]

rdar://problem/30352793

[John Wilander]

Created attachment 301570 [details]
Patch

[Alex Christensen]

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

[Sam Weinig]

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.

[John Wilander]

Created attachment 301858 [details]
Patch

[John Wilander]

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.

[John Wilander]

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.

[John Wilander]

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.

[John Wilander]

Created attachment 301862 [details]
Patch

[John Wilander]

Fixed my #if:s into #ifdef:s for HAVE_CORE_PREDICTION.

[Sam Weinig]

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.

[John Wilander]

Created attachment 301879 [details]
Patch

[John Wilander]

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

[John Wilander]

Created attachment 301882 [details]
Patch

[John Wilander]

Created attachment 301887 [details]
Patch

[John Wilander]

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

[Alexey Proskuryakov]

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

[John Wilander]

Created attachment 302014 [details]
Patch

[John Wilander]

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

[John Wilander]

Created attachment 302018 [details]
Patch

[WebKit Commit Bot]

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.

[John Wilander]

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

[John Wilander]

Created attachment 302022 [details]
Patch

[John Wilander]

(In reply to comment #23)
> Created attachment 302022 [details]
> Patch

svm_load_model() was missing from the re-declaration in the SPI file.

[WebKit Commit Bot]

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.

[Brent Fulgham]

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());

[John Wilander]

Created attachment 302172 [details]
Patch

[John Wilander]

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

[WebKit Commit Bot]

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.

[Alex Christensen]

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.

[John Wilander]

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

[WebKit Commit Bot]

Comment on attachment 302172 [details]
Patch

Clearing flags on attachment: 302172

Committed r212685: <http://trac.webkit.org/changeset/212685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r212685 for reason:

This change broke the 32-bit Sierra build.

Committed r212691: <http://trac.webkit.org/changeset/212691>

[John Wilander]

Created attachment 302281 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

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.

[John Wilander]

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.

[WebKit Commit Bot]

Comment on attachment 302281 [details]
Patch

Clearing flags on attachment: 302281

Committed r212757: <http://trac.webkit.org/changeset/212757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Wilander]

Committed r212800: <http://trac.webkit.org/changeset/212800>

[John Wilander]

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

[John Wilander]

Reopening to attach new patch.

[John Wilander]

Created attachment 302473 [details]
Patch

[WebKit Commit Bot]

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.

[John Wilander]

The previous patch was rolled out in https://trac.webkit.org/changeset/212841.

[John Wilander]

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().

[Brent Fulgham]

It would be great if David could double-check that we did the SOFT_LINK stuff properly.

[Brent Fulgham]

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.

[Brent Fulgham]

I can confirm that the release-mode crashes I was getting with the older patch are resolved with this new version.

[Alex Christensen]

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.

[John Wilander]

Created attachment 302626 [details]
Patch

[John Wilander]

Created attachment 302640 [details]
Patch

[John Wilander]

Created attachment 302641 [details]
Patch

[Alex Christensen]

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?

[Alex Christensen]

Comment on attachment 302641 [details]
Patch

I'm going to clean this up a bit.

[John Wilander]

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

[Alex Christensen]

Created attachment 302655 [details]
Patch

[Alex Christensen]

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

[John Wilander]

(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).

[WebKit Commit Bot]

Comment on attachment 302655 [details]
Patch

Clearing flags on attachment: 302655

Committed r212949: <http://trac.webkit.org/changeset/212949>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.