We've found a cash on espn.go.com. The reason is: 1. StringHash::equal assumes String::characters() is aligned to 4-byte boundary. 2. When PassRefPtr<StringImpl> StringImpl::create(const JSC::UString& str) uses shared buffer with UString, m_data is not guaranteed to be 4-byte aligned. because UString::Rep::data() can point to any offset of the internal buffer. The solution that Dave Tapuska suggests is: When UString::data() is not aligned to 4-byte, we just don't use the shared buffer. Anyone please give some comments?
> The solution that Dave Tapuska suggests is: When UString::data() is not aligned > to 4-byte, we just don't use the shared buffer. > > Anyone please give some comments? Tricky. I created this bug unfortunately. I can see at least two solutions: 1. Dave Tapuska;s suggestion. 2. Change StringHash::Equal to use memcmp You could try each solution separately in a ship build and run drameo and see which has less of a perf impact. I suspect that #1 is the better option.
(In reply to comment #1) > > The solution that Dave Tapuska suggests is: When UString::data() is not aligned > > to 4-byte, we just don't use the shared buffer. > > > > Anyone please give some comments? > > Tricky. I created this bug unfortunately. > > I can see at least two solutions: > 1. Dave Tapuska;s suggestion. > 2. Change StringHash::Equal to use memcmp > > You could try each solution separately in a ship build and run drameo and see > which has less of a perf impact. > > I suspect that #1 is the better option. Yeah, that's what we tried, and either way can fix the problem. But we haven't run any performance test so far. BTW, the performance affect may rely on platform/compiler. memcmp (or wmemcmp?) could probably be optimized with some inline code by compiler.
It's easy to make an alternate StringHash::equal in a way that does not depend on type punning in a non-portable way. The performance optimization of comparing four bytes at a time is not needed just to have WebKit. So I suggest starting by changing StringHash::equal to only do the optimization on platforms where it is safe. The other question, though, is whether the unaligned strings are having some sort of performance cost on platforms where the optimization is safe.
This thread discusses the issue, but it seems their conclusion was never adopted: http://lists.macosforge.org/pipermail/webkit-dev/2009-July/008630.html
Another thing about StringImpl (not related to this bug): newUCharVector() is defined, but never used. deleteUCharVector() is used, but I don't think it ever gets called because m_data is never a standalone memory block. So need some cleanup. Probably I should raise another bug for that?
Created attachment 43204 [details] Apply defines to StringHash::Equal
Comment on attachment 43204 [details] Apply defines to StringHash::Equal Rejecting patch 43204 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11623 test cases. http/tests/xmlhttprequest/workers/shared-worker-close.html -> crashed Exiting early after 1 failures. 9309 tests run. 363.31s total testing time 9308 test cases (99%) succeeded 1 test case (<1%) crashed 5 test cases (<1%) had stderr output
Comment on attachment 43204 [details] Apply defines to StringHash::Equal Clearing flags on attachment: 43204 Committed r51006: <http://trac.webkit.org/changeset/51006>
All reviewed patches have been landed. Closing bug.
*** Bug 31726 has been marked as a duplicate of this bug. ***
Cherry-picked into QtWebKit for Qt 4.6 with commit e3dc4ef2b801d91e115c54f833fa7766d392ceda