Bug 26057 – StringImpl should share buffers with UString.

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[David Levin]

see summary.

[David Levin]

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).

[David Levin]

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.

[David Levin]

Created attachment 30730 [details]
Proposed fix.

Fixed some comment issues noted by bdash.

[David Levin]

Adding mjs because I've been told he may be a good candidate to review this.

[Maciej Stachowiak]

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.

[Darin Adler]

(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.

[David Levin]

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

[David Levin]

Created attachment 30773 [details]
Proposed fix.

I believe that I've addressed all feedback.

[Darin Adler]

Comment on attachment 30773 [details]
Proposed fix.

r=me

[David Levin]

http://trac.webkit.org/changeset/44325