From 2119fe1b65d4ec8c50d1de55db4fa2939dedfb64 Mon Sep 17 00:00:00 2001 From: Geoffrey Garen Date: Fri, 21 Oct 2005 18:43:11 +0000 Subject: [PATCH] Patch by TimO, Reviewed by hyatt, tested and landed by me. Found what appears to be a misguided optimization that actually causes a measurable performance problem. A fixed-size buffer was allocated on the stack to pass into CFURLGetBytes(), presumably to avoid malloc() for URLs less than 2048 bytes. There was also a fallback which malloc()'ed a buffer in case the fixed-size buffer was too small to hold the URL's bytes. This malloc()'ed buffer was then wrapped in an NSData using +dataWithBytesNoCopy:length:, avoiding a memory copy (yay!) The problem with this approach is two-fold: 1. Regardless of how the buffer was allocated and filled, it is immediately wrapped in an NSData using +dataWithBytes:length:, which copies the input bytes. This is pretty much unavoidable; we need to get the data into a malloc()'ed buffer to return it to the caller, unless the caller provides its own storage (which would be super inconvenient). 2. The size of the fixed buffer was large enough that it fit most (if not all) URLs involved in our Page Load Test. This means the unintentionally-inefficient case was by far the most *common* case! My fix is to malloc() the buffer from the start, and then use +[NSData dataWithBytes:length:freeWhenDone:] to wrap the buffer in an NSData. This avoids a memory copy for the normal case where a URL is less than 2048 bytes, and keeps the efficient behavior for the uncommon long URL case. * Misc.subproj/WebNSURLExtras.m: (-[NSURL _web_originalData]): Canonical link: https://commits.webkit.org/9219@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@10882 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- WebKit/ChangeLog | 27 +++++++++++++++++++++++++++ WebKit/Misc.subproj/WebNSURLExtras.m | 23 +++++++++-------------- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/WebKit/ChangeLog b/WebKit/ChangeLog index 809c3abed019..03aa54373775 100644 --- a/WebKit/ChangeLog +++ b/WebKit/ChangeLog @@ -1,3 +1,30 @@ +2005-10-21 Geoffrey Garen + + Patch by TimO, Reviewed by hyatt, tested and landed by me. + + Found what appears to be a misguided optimization that actually causes a measurable performance problem. + A fixed-size buffer was allocated on the stack to pass into CFURLGetBytes(), presumably to avoid malloc() + for URLs less than 2048 bytes. There was also a fallback which malloc()'ed a buffer in case the fixed-size + buffer was too small to hold the URL's bytes. This malloc()'ed buffer was then wrapped in an NSData using + +dataWithBytesNoCopy:length:, avoiding a memory copy (yay!) + + The problem with this approach is two-fold: + + 1. Regardless of how the buffer was allocated and filled, it is immediately wrapped in an NSData using + +dataWithBytes:length:, which copies the input bytes. This is pretty much unavoidable; we need to get + the data into a malloc()'ed buffer to return it to the caller, unless the caller provides its own storage + (which would be super inconvenient). + + 2. The size of the fixed buffer was large enough that it fit most (if not all) URLs involved in our Page + Load Test. This means the unintentionally-inefficient case was by far the most *common* case! + + My fix is to malloc() the buffer from the start, and then use +[NSData dataWithBytes:length:freeWhenDone:] + to wrap the buffer in an NSData. This avoids a memory copy for the normal case where a URL is less than + 2048 bytes, and keeps the efficient behavior for the uncommon long URL case. + + * Misc.subproj/WebNSURLExtras.m: + (-[NSURL _web_originalData]): + 2005-10-21 Mitz Pettel Reviewed and landed by Darin. diff --git a/WebKit/Misc.subproj/WebNSURLExtras.m b/WebKit/Misc.subproj/WebNSURLExtras.m index 4d7a51221fae..6a77e4a7dabe 100644 --- a/WebKit/Misc.subproj/WebNSURLExtras.m +++ b/WebKit/Misc.subproj/WebNSURLExtras.m @@ -382,28 +382,23 @@ + (NSURL *)_web_URLWithData:(NSData *)data relativeToURL:(NSURL *)baseURL - (NSData *)_web_originalData { - NSData *data = nil; - - UInt8 static_buffer[URL_BYTES_BUFFER_LENGTH]; - CFIndex bytesFilled = CFURLGetBytes((CFURLRef)self, static_buffer, URL_BYTES_BUFFER_LENGTH); - if (bytesFilled != -1) { - data = [NSData dataWithBytes:static_buffer length:bytesFilled]; - } - else { + UInt8 *buffer = (UInt8 *)malloc(URL_BYTES_BUFFER_LENGTH); + CFIndex bytesFilled = CFURLGetBytes((CFURLRef)self, buffer, URL_BYTES_BUFFER_LENGTH); + if (bytesFilled == -1) { CFIndex bytesToAllocate = CFURLGetBytes((CFURLRef)self, NULL, 0); - UInt8 *buffer = malloc(bytesToAllocate); + buffer = (UInt8 *)realloc(buffer, bytesToAllocate); bytesFilled = CFURLGetBytes((CFURLRef)self, buffer, bytesToAllocate); ASSERT(bytesFilled == bytesToAllocate); - // buffer is adopted by the NSData - data = [NSData dataWithBytesNoCopy:buffer length:bytesFilled]; } - + + // buffer is adopted by the NSData + NSData *data = [NSData dataWithBytesNoCopy:buffer length:bytesFilled freeWhenDone:YES]; + NSURL *baseURL = (NSURL *)CFURLGetBaseURL((CFURLRef)self); if (baseURL) { NSURL *URL = [NSURL _web_URLWithData:data relativeToURL:baseURL]; return [URL _web_originalData]; - } - else { + } else { return data; } }