Bug 25126 - UString should allow its buffer to be shared.
: UString should allow its buffer to be shared.
: WebKit
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: 23175
  Show dependency treegraph
Reported: 2009-04-09 16:36 PST by
Modified: 2009-05-25 21:21 PST (History)

Proposed fix. (22.01 KB, patch)
2009-04-09 17:19 PST, David Levin
no flags Review Patch | Details | Formatted Diff | Diff
Proposed fix. (26.56 KB, patch)
2009-05-25 19:25 PST, David Levin
mjs: review+
Review Patch | Details | Formatted Diff | Diff


You need to log in before you can comment on or make changes to this bug.

Description From 2009-04-09 16:36:34 PST
See summary.
------- Comment #1 From 2009-04-09 17:19:26 PST -------
Created an attachment (id=29381) [details]
Proposed fix.
------- Comment #2 From 2009-04-23 16:55:50 PST -------
(From update of attachment 29381 [details])
 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.
------- Comment #3 From 2009-05-22 02:44:27 PST -------
(From update of attachment 29381 [details])
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
------- Comment #4 From 2009-05-22 11:04:23 PST -------
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.)
------- Comment #5 From 2009-05-22 15:09:58 PST -------
(From update of attachment 29381 [details])
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.
------- Comment #6 From 2009-05-25 19:25:45 PST -------
Created an attachment (id=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).
------- Comment #7 From 2009-05-25 20:06:49 PST -------
(From update of attachment 30661 [details])

Thanks for checking performance!
------- Comment #8 From 2009-05-25 21:21:58 PST -------