Bug 25126 – UString should allow its buffer to be shared.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Levin]

See summary.

[David Levin]

Created attachment 29381 [details]
Proposed fix.

[Eric Seidel (no email)]

Comment on attachment 29381 [details]
Proposed fix.

 335         PassRefPtr<SharedUChar> sharedBuffer() { return m_rep->baseString()->sharedBuffer(); }

Should just return a raw pointer, right?  I don't see why it would need to return a PassRefPtr here.  I guess the idea is that since the UString doesn't hold it in a RefPtr, we should force the caller to?

I think new code should use clearer variable names:
 235 PassRefPtr<UString::Rep> UString::Rep::share(UChar* d, int l, PassRefPtr<UString::SharedUChar> sharedBuffer)
to match our modern style guidelines (which this class predates by forever :)

     // It make doesn't sense to share < 6 bytes of data.
 386     ASSERT(len > 2);
Agreed.  But how do we come up with this heuristic?

releaseRef is not needed here:
 396     m_sharedBuffer = sharedBuffer.releaseRef();

At least if m_sharedBuffer is a RefPtr.  I'm confused as to why it wouldn't be.  If it shouldn't be that at least deserves a changelog comment. :)

I guess you named OwnMallocPtr with Malloc to differentiate it from OwnPtr which calls delete.  However, it seems odd that OwnMallocPtr actually calls fastFree instead of free.  Seems it should be OwnFastMallocPtr in that case.

Otherwise I think the general idea is fine.  I expect that Darin Adler will want to weigh in here eventually.

[Oliver Hunt]

Comment on attachment 29381 [details]
Proposed fix.

In the absence of any other reviewers i've gone through this and think it looks good.  Alas the delay in reviewing means we should retest performance before landing

[David Levin]

Thanks Oliver.

Ok, I'll re-perf test it before landing.  Also, I'll address Eric's comments.  (Lastly, it doesn't apply cleanly right now due to changes in the code, so I'll fix that as well of course.)
 

[David Levin]

Comment on attachment 29381 [details]
Proposed fix.

I'm removing the r+ for now, so this doesn't get landed.  I've done some perf test and I see a regression that I need to investigate.

I'll resubmit once I can figure that out.

[David Levin]

Created attachment 30661 [details]
Proposed fix.

The only changes from the last patch were:
1. Addressing previous given feedback and catching up with recent changes to UString.*
2. I switch the conditional around in Rep::baseString which (helped branch prediction enough to) made the performance equivalent to what it was before on Sunspider.  So there is no change in Sunspider results (as noted by running it 1000 times with and without the change).

[Maciej Stachowiak]

Comment on attachment 30661 [details]
Proposed fix.

r=me

Thanks for checking performance!

[David Levin]

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