Bug 144604 - [Content Extensions] Use less memory to store the json input
Summary: [Content Extensions] Use less memory to store the json input
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-04 16:00 PDT by Alex Christensen
Modified: 2015-05-05 15:22 PDT (History)
1 user (show)

See Also:


Attachments
Patch (4.04 KB, patch)
2015-05-04 16:03 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (11.58 KB, patch)
2015-05-05 11:52 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (13.24 KB, patch)
2015-05-05 12:06 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (8.49 KB, patch)
2015-05-05 15:13 PDT, Alex Christensen
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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