RESOLVED FIXED 33943
Use fastStrDup instead of strdup
https://bugs.webkit.org/show_bug.cgi?id=33943
Summary Use fastStrDup instead of strdup
Kwang Yul Seo
Reported 2010-01-21 02:44:40 PST
fastStrDup is added to FastMalloc in https://bugs.webkit.org/show_bug.cgi?id=33937 Replace strdup/free with fastStrDup/fastFree.
Attachments
Use fastStrDup instead of strdup (8.87 KB, patch)
2010-01-21 02:55 PST, Kwang Yul Seo
darin: review-
darin: commit-queue-
Use fastStrDup (8.88 KB, patch)
2010-01-22 05:47 PST, Kwang Yul Seo
eric: review-
commit-queue: commit-queue-
Patch (8.02 KB, patch)
2010-02-03 07:39 PST, Kwang Yul Seo
no flags
Kwang Yul Seo
Comment 1 2010-01-21 02:55:14 PST
Created attachment 47107 [details] Use fastStrDup instead of strdup
WebKit Review Bot
Comment 2 2010-01-21 03:00:41 PST
WebKit Review Bot
Comment 3 2010-01-21 03:00:51 PST
Kwang Yul Seo
Comment 4 2010-01-21 03:02:55 PST
The following files still contain strdup after this patch. - bindings/v8/npruntime.cpp - bridge/npruntime.cpp - bindings/v8/V8NPUtils.cpp NPN_UTF8FromIdentifier uses strdup to duplicate a string, so the returned memory must be freed with NPN_MemFree. As NPN_MemFree is implemented with free, strdup can't be replaced with fastStrDup. Technically speaking, we can change NPN_MemAlloc/NPN_MemFree to use fastMalloc/fastFree internally and use fastStrDup in NPN_UTF8FromIdentifier. However, some plugins use free instead of NPN_MemFree, replacing strdup with fastStrDup breaks those plugins. NPN_ReleaseVariantValue has the same issue as NPN_UTF8FromIdentifier.
WebKit Review Bot
Comment 5 2010-01-21 03:03:06 PST
Kwang Yul Seo
Comment 6 2010-01-21 03:07:10 PST
Build tests won't pass until 33937 is committed.
Darin Adler
Comment 7 2010-01-21 10:01:15 PST
Comment on attachment 47107 [details] Use fastStrDup instead of strdup > Index: WebCore/bridge/jni/JNIBridge.cpp > =================================================================== > --- WebCore/bridge/jni/JNIBridge.cpp (revision 53619) > +++ WebCore/bridge/jni/JNIBridge.cpp (working copy) > @@ -301,7 +301,7 @@ JavaMethod::JavaMethod(JNIEnv* env, jobj > JavaMethod::~JavaMethod() > { > if (m_signature) > - free(m_signature); > + fastFree(m_signature); > delete[] m_parameters; > }; We could change this to be an OwnPtr now. Also, there is never a need for a null check on a fastFree call. > @@ -311,7 +311,7 @@ static void appendClassName(StringBuilde > { > ASSERT(JSLock::lockCount() > 0); > > - char* c = strdup(className); > + char* c = fastStrDup(className); Unfortunate for performance that this is always on the heap. It would be better if this used Vector<char, 64> or something like that. > JavaArray::~JavaArray() > { > - free(const_cast<char*>(m_type)); > + fastFree(const_cast<char*>(m_type)); > } We could change this to be an OwnPtr now. > @@ -409,7 +409,7 @@ void JavaArray::setValueAt(ExecState* ex > // The type of the array will be something like: > // "[Ljava.lang.string;". This is guaranteed, so no need > // for extra sanity checks. > - javaClassName = strdup(&m_type[2]); > + javaClassName = fastStrDup(&m_type[2]); > javaClassName[strchr(javaClassName, ';')-javaClassName] = 0; > } > jvalue aJValue = convertValueToJValue(exec, aValue, arrayType, javaClassName); > @@ -472,7 +472,7 @@ void JavaArray::setValueAt(ExecState* ex > } > > if (javaClassName) > - free(const_cast<char*>(javaClassName)); > + fastFree(const_cast<char*>(javaClassName)); > } We could change this to be an OwnPtr now. > JavaClass::~JavaClass() > { > - free(const_cast<char*>(m_name)); > + fastFree(const_cast<char*>(m_name)); We could change this to be an OwnPtr now. > - free(m_url); > + fastFree(m_url); We could change this to be an OwnPtr now. > Index: WebCore/platform/network/curl/ResourceHandleManager.cpp > =================================================================== > --- WebCore/platform/network/curl/ResourceHandleManager.cpp (revision 53619) > +++ WebCore/platform/network/curl/ResourceHandleManager.cpp (working copy) > @@ -135,13 +135,13 @@ ResourceHandleManager::~ResourceHandleMa > curl_multi_cleanup(m_curlMultiHandle); > curl_share_cleanup(m_curlShareHandle); > if (m_cookieJarFileName) > - free(m_cookieJarFileName); > + fastFree(m_cookieJarFileName); > curl_global_cleanup(); > } We could change this to be an OwnPtr now. > // url is in ASCII so latin1() will only convert it to char* without character translation. > - d->m_url = strdup(url.latin1().data()); > + d->m_url = fastStrDup(url.latin1().data()); > curl_easy_setopt(d->m_handle, CURLOPT_URL, d->m_url); I don't see the change to the free call here. Is this introducing a mismatched malloc/free pair? review- because the try bots say this breaks the build
Kwang Yul Seo
Comment 8 2010-01-21 14:57:26 PST
d->m_url is freed in ResourceHandleInternal::~ResourceHandleInternal, so there is no mismatched malloc/free pair. We could change this to be an OwnPtr too. I will update the patch as you suggested.
Darin Adler
Comment 9 2010-01-21 23:17:38 PST
All that stuff I said about OwnPtr is wrong. OwnPtr does a delete, not a free. So if we wanted to use a smart pointer we'd either need to use a new one or use OwnArrayPtr and use something like strDup that did new char[] instead of fastMalloc. Oof. The strdup function isn't so great.
Kwang Yul Seo
Comment 10 2010-01-22 05:47:53 PST
Created attachment 47194 [details] Use fastStrDup fastStrDup is not so great as we can't use OwnPtr. However, it is still valuable because we can use fastFree everywhere. In addition to it, some compilers without strdup can benefit from fastStrDup. I submit the patch again to make it pass build tests as 33943 is now committed in the trunk.
Kwang Yul Seo
Comment 11 2010-01-22 05:49:21 PST
I will file a separate bug for using Vector<char, 64> to avoid a heap allocation.
WebKit Commit Bot
Comment 12 2010-01-22 11:19:01 PST
Comment on attachment 47194 [details] Use fastStrDup Rejecting patch 47194 from commit-queue. Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1 Last 500 characters of output: C8Bindings10JavaMethod9signatureEv in JNIBridge.o __ZN3JSC8Bindings9JavaArrayC1EP8_jobjectPKcN3WTF10PassRefPtrINS0_10RootObjectEEE in JNIBridge.o __ZN3JSC8Bindings9JavaArrayC2EP8_jobjectPKcN3WTF10PassRefPtrINS0_10RootObjectEEE in JNIBridge.o ld: symbol(s) not found collect2: ld returned 1 exit status ** BUILD FAILED ** The following build commands failed: WebCore: Ld /Users/eseidel/Projects/CommitQueue/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore normal i386 (1 failure) Full output: http://webkit-commit-queue.appspot.com/results/204912
Eric Seidel (no email)
Comment 13 2010-02-02 14:00:05 PST
Comment on attachment 47194 [details] Use fastStrDup Looks like this would break the mac build. r-
Kwang Yul Seo
Comment 14 2010-02-03 07:39:16 PST
Created attachment 48030 [details] Patch Update the patch. The build failed because WTF::fastStrDup is not exported. I filed a separate bug for this in https://bugs.webkit.org/show_bug.cgi?id=34526
Kwang Yul Seo
Comment 15 2010-02-08 08:44:38 PST
As bug 34526 is now landed, this needs to reviewed again.
WebKit Commit Bot
Comment 16 2010-02-08 10:22:06 PST
Comment on attachment 48030 [details] Patch Clearing flags on attachment: 48030 Committed r54495: <http://trac.webkit.org/changeset/54495>
WebKit Commit Bot
Comment 17 2010-02-08 10:22:14 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.