Bug 26057 - StringImpl should share buffers with UString.
Summary: StringImpl should share buffers with UString.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks: 23175
  Show dependency treegraph
 
Reported: 2009-05-27 23:28 PDT by David Levin
Modified: 2009-06-01 11:50 PDT (History)
2 users (show)

See Also:


Attachments
Proposed fix. (15.80 KB, patch)
2009-05-28 00:01 PDT, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix. (15.57 KB, patch)
2009-05-28 01:47 PDT, David Levin
mjs: review-
Details | Formatted Diff | Diff
Proposed fix. (15.14 KB, patch)
2009-05-29 03:45 PDT, David Levin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-05-27 23:28:32 PDT
see summary.
Comment 1 David Levin 2009-05-28 00:01:20 PDT
Created attachment 30729 [details]
Proposed fix.

Some perf test results:

On http://www.hixie.ch/tests/adhoc/perf/dom/artificial/core/001.html
the time went from 78ms to 40ms for append (other times remained constant).

On http://www.hixie.ch/tests/adhoc/perf/dom/artificial/core/002.html,
the time went from 3900ms to 2600ms.

For http://dromaeo.com/?dom, the time for DomModification improved by ~6%.  Other tests in dom seemed to be faster across several runs but within the margin of error (except DOM Attributes which was slightly ~1.5% worse).
Comment 2 David Levin 2009-05-28 00:27:39 PDT
Adding Oliver to tempt him to review it since he review the UString portion before.  Also this patch will allow WorkerContext::scriptImported to be finished.
Comment 3 David Levin 2009-05-28 01:47:37 PDT
Created attachment 30730 [details]
Proposed fix.

Fixed some comment issues noted by bdash.
Comment 4 David Levin 2009-05-28 01:48:31 PDT
Adding mjs because I've been told he may be a good candidate to review this.
Comment 5 Maciej Stachowiak 2009-05-28 04:17:25 PDT
Love the cited perf results. Some comments below:

I'd consider citing the performance results in the ChangeLog.

I don't think you need quite this long a comment to explain the exact value. It seems fine to just mention the mandatory minimum here.

+// This number must be at least 1 to avoid sharing empty, null and 1 character strings from SmallStrings.
+// Doing the sharing cost 12 bytes, so it seemed reasonable that the number of bytes should be larger than that.
+// After testing with dom benchmarks like http://dromaeo.com/?dom, it seems that a relatively low number gives
+// good performance results.
+static const int minLengthToShare = 30;


This comment is also lengthy, and in addition false - the values are 30 and 20 so they are not the same.

+// This number must be at least 1 to avoid sharing empty, null and 1 character strings from SmallStrings.
+// Doing the sharing cost 12 bytes, so it seemed reasonable that the number of bytes should be larger than that.
+// After testing with dom benchmarks like http://dromaeo.com/?dom, it seems that a relatively low number gives
+// good performance results.  (This number may vary independently from the value in JavaScriptCore/runtime/Ustring.cpp
+// but they happen to have the same value at present.)
+static const unsigned minLengthToShare = 20;



share() seems like a subtle name for such a simple method. I wonder if a more explicit name like createSharedCopy() might be better. I'm not entirely sure though - just something to consider.

+PassRefPtr<StringImpl> StringImpl::share(const JSC::UString& str)



 PassRefPtr<StringImpl> StringImpl::copy() is meant to explicitly make a copy, for example for crazy cases where someone may poke directly into the buffer. I am not sure it's sound to make it share the buffer instead. How do we know this is safe?


The only issue I am seriously concerned about is the copy() thing - the rest are minor and optional to deal with. r- to revert the copy() part of the change or explain clearly why it is sound.
Comment 6 Darin Adler 2009-05-28 07:46:57 PDT
(In reply to comment #5)
>  PassRefPtr<StringImpl> StringImpl::copy() is meant to explicitly make a copy,
> for example for crazy cases where someone may poke directly into the buffer. I
> am not sure it's sound to make it share the buffer instead. How do we know this
> is safe?

I believe currently it's used for cross-thread cases where we intentionally don't want to try to share the reference count. I don't think there are any "poke directly into the buffer" cases. But sharing a single reference count in this case would not be safe.

Since StringImpl's are immutable, we only use copy() when we have a cross-thread issue. Maybe eventually we can get rid of it entirely.
Comment 7 David Levin 2009-05-29 01:36:36 PDT
I found every usage (by changing the function name and compiling) of String::copy() and looked at them.  All of them are for cross thread usage.  

However, it was a good thing to review all of these because I didn't realize that some of them call the copy() method the first time on a different thread.  This is safe using the previous implementation (although you have to be *very* careful to not accidentally ref count the string on the other thread) but not with my current implementation so I'll revert this implementation of copy().

As a reference, here's all the copy method calls and comments about any unusual aspects.

// Storage related
WebCore/storage/ChangeVersionWrapper.cpp:37,38
WebCore/storage/DatabaseTracker.cpp:793
WebCore/storage/Database.cpp:130, 240, 276, 475, 579, 604, 615 -- Used for another thread *but* the copy is done on the other thread (not the owning thread).
WebCore/storage/LocalStorage.cpp:64
WebCore/storage/LocalStorageArea.cpp:284 
WebCore/storage/OriginQuotaManager.cpp:82
WebCore/storage/SQLStatement.cpp:53
WebCore/storage/SQLTransaction.cpp:99 -- Does an unnecessary copy -- Filed https://bugs.webkit.org/show_bug.cgi?id=26074.
WebCore/storage/SQLError.h:44,47 -- x-thread *but* the copy is done on the other thread (not the owning thread).
WebCore/storage/StorageMap.cpp:152,155
WebCore/platform/sql/SQLValue.cpp:37,46

// x-thread copy support for these structures
WebCore/platform/KURL.cpp:534
WebCore/page/SecurityOrigin.cpp:83,84,85
WebCore/platform/network/HTTPHeaderMap.cpp:48
WebCore/platform/network/ResourceErrorBase.cpp:37,39,40
WebCore/platform/network/ResourceRequestBase.cpp:76,82
WebCore/platform/network/ResourceResponseBase.cpp:65,67,68,70

// misc
WebCore/loader/icon/IconDatabase.cpp:129,228,264,313,322,406,414,489,513,590,591,848,854
WebCore/dom/MessagePort.cpp:76

// Worker related
WebCore/loader/WorkerThreadableLoader.cpp:99
WebCore/loader/CrossOriginPreflightResultCache.cpp:148
WebCore/platform/CrossThreadCopier.cpp:44
WebCore/workers/WorkerMessagingProxy.cpp:54,81,109,111
WebCore/workers/WorkerRunLoop.cpp:80,205
WebCore/workers/WorkerThread.cpp:60,61
Comment 8 David Levin 2009-05-29 03:45:36 PDT
Created attachment 30773 [details]
Proposed fix.

I believe that I've addressed all feedback.
Comment 9 Darin Adler 2009-06-01 00:26:13 PDT
Comment on attachment 30773 [details]
Proposed fix.

r=me
Comment 10 David Levin 2009-06-01 11:50:16 PDT
http://trac.webkit.org/changeset/44325