Bug 25126 - UString should allow its buffer to be shared.
Summary: UString should allow its buffer to be shared.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks: 23175
  Show dependency treegraph
 
Reported: 2009-04-09 16:36 PDT by David Levin
Modified: 2009-05-25 21:21 PDT (History)
2 users (show)

See Also:


Attachments
Proposed fix. (22.01 KB, patch)
2009-04-09 17:19 PDT, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix. (26.56 KB, patch)
2009-05-25 19:25 PDT, David Levin
mjs: 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-04-09 16:36:34 PDT
See summary.
Comment 1 David Levin 2009-04-09 17:19:26 PDT
Created attachment 29381 [details]
Proposed fix.
Comment 2 Eric Seidel (no email) 2009-04-23 16:55:50 PDT
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.
Comment 3 Oliver Hunt 2009-05-22 02:44:27 PDT
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
Comment 4 David Levin 2009-05-22 11:04:23 PDT
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 David Levin 2009-05-22 15:09:58 PDT
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.
Comment 6 David Levin 2009-05-25 19:25:45 PDT
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).
Comment 7 Maciej Stachowiak 2009-05-25 20:06:49 PDT
Comment on attachment 30661 [details]
Proposed fix.

r=me

Thanks for checking performance!
Comment 8 David Levin 2009-05-25 21:21:58 PDT
http://trac.webkit.org/changeset/44145