Bug 144604

Summary: [Content Extensions] Use less memory to store the json input
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: WebCore Misc.Assignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch benjamin: review+

Description Alex Christensen 2015-05-04 16:00:33 PDT
We have several copies of the JSON data that is an input to the content extension compiler.  We need to copy it at least once to convert the utf8 characters to unicode characters for the JSON parser (until we make the JSON parser nicer), but we can free that memory as soon as we are done with it to reduce the peak memory usage.
Comment 1 Alex Christensen 2015-05-04 16:03:07 PDT
Created attachment 252346 [details]
Patch
Comment 2 Darin Adler 2015-05-04 16:06:10 PDT
Comment on attachment 252346 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252346&action=review

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm:61
> +    encodedContentExtension = nil; // shouldn't be necessary.

This is wrong. This is not ARC, so you need to explicitly call release. Setting to nil won’t do anything at all. This will simply cause the data to leak.
Comment 3 Alex Christensen 2015-05-05 11:52:10 PDT
Created attachment 252394 [details]
Patch
Comment 4 Alex Christensen 2015-05-05 12:06:38 PDT
Created attachment 252396 [details]
Patch
Comment 5 Alex Christensen 2015-05-05 12:32:34 PDT
I imagine I don't need to retain the NSStrings in _WKUserContentExtensionStoreTest because they are static, right?
Will I need to simultaneously change any code in any programs that call compileContentExtensionForIdentifier?  What about ARC code that calls compileContentExtensionForIdentifier?
Comment 6 Benjamin Poulain 2015-05-05 15:02:26 PDT
Comment on attachment 252396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252396&action=review

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.h:37
> -- (void)compileContentExtensionForIdentifier:(NSString *)identifier encodedContentExtension:(NSString *)encodedContentExtension completionHandler:(void (^)(_WKUserContentFilter *, NSError *))completionHandler;
> +- (void)compileContentExtensionForIdentifier:(NSString *)identifier encodedContentExtension:(NSString *) NS_RELEASES_ARGUMENT encodedContentExtension completionHandler:(void (^)(_WKUserContentFilter *, NSError *))completionHandler;

The patch looks like a good move, but I don't know about NS_RELEASES_ARGUMENT.
Does that work with ARC?
Comment 7 Alex Christensen 2015-05-05 15:13:09 PDT
Created attachment 252410 [details]
Patch
Comment 8 Alex Christensen 2015-05-05 15:14:16 PDT
Releasing the NSString should be done in another patch.  It's another small gain, but I'm not as familiar with ObjC reference counting.
Comment 9 Benjamin Poulain 2015-05-05 15:19:27 PDT
Comment on attachment 252410 [details]
Patch

lgtm
Comment 10 Alex Christensen 2015-05-05 15:22:11 PDT
http://trac.webkit.org/changeset/183832