Summary: | add 7 bit strings capabilities to the v8 binding layer | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dan Carney <dcarney> | ||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric, haraken, japhet, jochen, leviw, msaboff, ojan, pdr, sullivan, webkit.review.bot | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Dan Carney
2012-07-20 04:12:41 PDT
Created attachment 153483 [details]
Patch
Attachment 153483 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/v8/V8Binding.cpp:406: The parameter name "resource" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
Total errors found: 3 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 153485 [details]
Patch
Comment on attachment 153483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153483&action=review > Source/WebCore/ChangeLog:12 > + No new tests. Refactor - no new functionality. (I've not looked into the patch in detail but) would you check performance impact? We want to confirm that this change won't regress binding performance. You can test binding performance by WebKit/PerformanceTests/Dromaeo/dom-*.html and WebKit/PerformanceTests/Bindings/*. (You can add a new one if needed.) Created attachment 153506 [details]
typical performance test results without patch
Created attachment 153507 [details]
typical performance test results with patch
(In reply to comment #4) > (From update of attachment 153483 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153483&action=review > > > Source/WebCore/ChangeLog:12 > > + No new tests. Refactor - no new functionality. > > (I've not looked into the patch in detail but) would you check performance impact? We want to confirm that this change won't regress binding performance. You can test binding performance by WebKit/PerformanceTests/Dromaeo/dom-*.html and WebKit/PerformanceTests/Bindings/*. (You can add a new one if needed.) I've run the performance tests for you. No obvious degradation, but the one place I suspect there could be performance degradation String::containsOnlyASCII() I intend to fix upstream, so that the cost of calling that will be gone. (In reply to comment #7) > I've run the performance tests for you. No obvious degradation, Great! Is this just a noise? - without your patch: dom-attr= 7587.33618079 runs/s - with your patch: dom-attr= 7055.17692413 runs/s Anyway this patch looks really great. I believe this patch will definitely save memory but I'm not sure how much. It would be also helpful to investigate how much memory are saved by this patch in a couple of real-world websites (Google Docs, Google SpreadSheet, Gmail, facebook, twitter etc). Comment on attachment 153485 [details]
Patch
This patch is causing a large number of tests to fail.
(In reply to comment #10) > (From update of attachment 153485 [details]) > This patch is causing a large number of tests to fail. The errors come from v8 ascii handling. A bug has been filed with the v8 team. Created attachment 154067 [details]
Patch
reuploaded patch with bug fix from v8 Comment on attachment 154067 [details] Patch Attachment 154067 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13323635 Comment on attachment 154067 [details]
Patch
This is a big (wonderful) change. Before looking into details, I'd like to confirm a couple of things. Would you describe the following points in ChangeLog?
- Performance impact. (Was the dom-attr.html result just a noise? WebKit performance tests can just catch 10%~ regression. So it is important to manually make sure that a doubtful patch never regresses performance. Please check Bindings/* and Dromaeo/dom-*)
- Memory benefit. (How much memory is saved by this patch? Let's take a couple of real-world websites or artificial benchmarks and measure the memory benefit.)
- Test coverage. (You mentioned that existing tests are already testing the call path of this patch. Would you list up some of them instead of writing "No new tests. Refactor - no new functionality."?)
Looks like you need to update the V8 DEPS of chromium-ews. (You can ask it in IRC. If tomorrow is OK, I can do it.) (In reply to comment #16) > Looks like you need to update the V8 DEPS of chromium-ews. (You can ask it in IRC. If tomorrow is OK, I can do it.) The V8 update was reverted because it caused random crashes, so we'll have to wait a bit longer it seems :-/ Comment on attachment 154067 [details]
Patch
OK. For now marking r- due to the build failure. (We do not just want to list up r? patches in the review list.)
Created attachment 163109 [details]
patch for perfalizer
Attachment 163109 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/bindings/v8/V8PerIsolateDat..." exit_code: 1
Source/WebCore/bindings/v8/V8StringResource.cpp:143: An else should appear on the same line as the preceding } [whitespace/newline] [4]
Source/WebCore/bindings/v8/V8StringResource.cpp:134: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Source/WebCore/bindings/v8/V8ValueCache.h:146: Missing space inside { }. [whitespace/braces] [5]
Source/WebCore/bindings/v8/V8ValueCache.h:147: Missing space inside { }. [whitespace/braces] [5]
Source/WebCore/bindings/v8/V8ValueCache.h:164: Missing space inside { }. [whitespace/braces] [5]
Source/WebCore/bindings/v8/V8ValueCache.h:165: Missing space inside { }. [whitespace/braces] [5]
Total errors found: 6 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 163333 [details]
perfalizer patch
Created attachment 163334 [details]
perf results of 163333
Created attachment 169187 [details]
Patch
Comment on attachment 169187 [details] Patch Attachment 169187 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14412150 New failing tests: inspector/timeline/timeline-load.html Comment on attachment 169187 [details] Patch Attachment 169187 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14390604 New failing tests: inspector/timeline/timeline-load.html Comment on attachment 169187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169187&action=review > Source/WebCore/ChangeLog:11 > + No new tests. Test coverage already extensive. Since this should be a perf win, it seems appropriate to comment on how much of a win it is (in whatever example benchmarks you like) in the ChangeLog. > Source/WebCore/bindings/v8/V8StringResource.cpp:42 > + inline static String fromStringResource(WebCoreStringResourceBase* resource) Should this return const String&? it seems the resource always owns the String. > Source/WebCore/bindings/v8/V8StringResource.cpp:135 > return String(""); String::empty() is faster. :) > Source/WebCore/bindings/v8/V8ValueCache.cpp:44 > + if (string.is8Bit() && string.containsOnlyASCII()) { > + WebCoreStringResource8* stringResource = new WebCoreStringResource8(string); > + v8::Local<v8::String> newString = v8::String::NewExternal(stringResource); > + if (newString.IsEmpty()) > + delete stringResource; > + return newString; > + } > + > + WebCoreStringResource16* stringResource = new WebCoreStringResource16(string); Can't we share this block by using a WebCoreStringResourceBase* ? Or does it not have a virtual destructor? > Source/WebCore/bindings/v8/V8ValueCache.h:87 > + inline explicit WebCoreStringResourceBase(const AtomicString& string) I was under the impression that marking things "inline" in headers was meanlingless? I thought 'inline' was implied for anything which was listed as part of the class definition? (In reply to comment #26) > (From update of attachment 169187 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169187&action=review > > > Source/WebCore/bindings/v8/V8StringResource.cpp:135 > > return String(""); > > String::empty() is faster. :) But not thread safe. My understanding is that this code is used in workers as well. Created attachment 169406 [details]
Patch
I've cleaned up the patch a bit. It will fail until a v8 bug fix makes its way through the pipeline, but I'd still like another review so I can get this patch in the commit queue as soon as that happens. I have the feeling that this patch will get rolled back a few times, as it's using a bunch of v8 functionality never used before, and I'd like to work through that sooner rather than later. > Since this should be a perf win, it seems appropriate to comment on how much of a win it is (in whatever example benchmarks you like) in the ChangeLog. Okay, I forgot to add them in this patch, I'll add those in on the final submit. Some representative numbers are in https://bugs.webkit.org/attachment.cgi?id=163334, but like are perf numbers are probably somewhat wrong. In particular: Bindings/append-child Bindings/create-element Bindings/id-getter Dromaeo/jslib-style-jquery Dromaeo/jslib-attr-jquery Dromaeo/dromaeo-object-regexp Dromaeo/sunspider-crypto-md5 see some consistent improvement. There are a few loses but they seem less drastic than the gains, and I have future patches to v8 which should mitigate them. > > Source/WebCore/bindings/v8/V8ValueCache.cpp:44 > > + if (string.is8Bit() && string.containsOnlyASCII()) { > > + WebCoreStringResource8* stringResource = new WebCoreStringResource8(string); > > + v8::Local<v8::String> newString = v8::String::NewExternal(stringResource); > > + if (newString.IsEmpty()) > > + delete stringResource; > > + return newString; > > + } > > + > > + WebCoreStringResource16* stringResource = new WebCoreStringResource16(string); > > Can't we share this block by using a WebCoreStringResourceBase* ? Or does it not have a virtual destructor? It's not obvious here, but v8::String::NewExternal() is the problem as it's overloaded and doesn't accept ExternalStringBase. Comment on attachment 169406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169406&action=review > Source/WebCore/bindings/v8/V8PerIsolateData.cpp:116 > + if (encoding == v8::String::ASCII_ENCODING) > + base = static_cast<WebCoreStringResource8*>(resource); > + else > + base = static_cast<WebCoreStringResource16*>(resource); This code looks very strange. Why is it important to use the right static cast? Presumably WebCoreStringResourceBase is the only base class for WebCoreStringResource8 and WebCoreStringResource16, which means these static casts should be zero offsets. We can add an ASSERT to that effect. > Source/WebCore/bindings/v8/V8StringResource.cpp:82 > + static const int inlineBufferSize = 16; Is this constant related to something intrinsic to AtomicString? if so, can we get the constant from AtomicString rather than having to duplicate it here? > Source/WebCore/bindings/v8/V8StringResource.cpp:100 > + v8String->WriteAscii(reinterpret_cast<char*>(buffer), 0, length, v8::String::PRESERVE_ASCII_NULL); The NULL in this constant got me thinking. Are we getting things correct w.r.t. null termination? For example, the buffer in String isn't null terminated. If V8 is trying to write a null terminator, the lengths will be off by one. I guess that would show up in tests if we were goofing that up. > Source/WebCore/bindings/v8/V8StringResource.cpp:125 > + v8::String::Encoding encoding; > + v8::String::ExternalStringResourceBase* resource = v8String->GetExternalStringResourceBase(&encoding); > + if (LIKELY(!!resource)) { > + WebCoreStringResourceBase* base; > + if (encoding == v8::String::ASCII_ENCODING) > + base = static_cast<WebCoreStringResource8*>(resource); > + else > + base = static_cast<WebCoreStringResource16*>(resource); > + return StringTraits<StringType>::fromStringResource(base); > + } This code looks largely duplicated from V8PerIsolateData.cpp. Is there no way to share this logic? I also don't understand why we need this tricky static_cast logic (same question as above) > Source/WebCore/bindings/v8/V8StringResource.cpp:129 > - if (!length) > + if (UNLIKELY(!length)) Did you measure this UNLIKELY as having a performance benefit? It's most likely a no-op. > Source/WebCore/bindings/v8/V8ValueCache.h:151 > + return static_cast<WebCoreStringResource16*>(v8String->GetExternalStringResource()); Should we add an ASSERT about the encoding of v8String? > Source/WebCore/bindings/v8/V8ValueCache.h:154 > + explicit WebCoreStringResource16(const String& string): WebCoreStringResourceBase(string) { } > + explicit WebCoreStringResource16(const AtomicString& string): WebCoreStringResourceBase(string) { } nit: Space before : Comment on attachment 169406 [details]
Patch
I'm marking this r- because its causing a lot of tests to fail, but I actually think this is looking pretty good. My previous comments are mostly just details.
(If I were you, I'd add a link to https://bug-91850-attachments.webkit.org/attachment.cgi?id=163334 in the ChangeLog entry.) Some of those perf results show large swings in "bytes". Is that expected? Comment on attachment 169406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169406&action=review >> Source/WebCore/bindings/v8/V8PerIsolateData.cpp:116 >> + base = static_cast<WebCoreStringResource16*>(resource); > > This code looks very strange. Why is it important to use the right static cast? Presumably WebCoreStringResourceBase is the only base class for WebCoreStringResource8 and WebCoreStringResource16, which means these static casts should be zero offsets. We can add an ASSERT to that effect. The hierarchy is a little wierd here because we computing offsets between 2 different base classes with respect to 4 subclasses. I expect the offsets are the same because of the similarity of ExternalAsciiStringResource and ExternalStringResource, but the offsets are definitely not 0. I can add an ASSERT to verify the sameness, but I was assuming the compiler would remove the branch if they happened to be equal. >> Source/WebCore/bindings/v8/V8StringResource.cpp:82 >> + static const int inlineBufferSize = 16; > > Is this constant related to something intrinsic to AtomicString? if so, can we get the constant from AtomicString rather than having to duplicate it here? I didn't write this, but I expect there is a performance gain up to a point doing everything on the stack, and the original writer just guessed at a number that sounded reasonable. It has nothing to do with AtomicString. >> Source/WebCore/bindings/v8/V8StringResource.cpp:100 >> + v8String->WriteAscii(reinterpret_cast<char*>(buffer), 0, length, v8::String::PRESERVE_ASCII_NULL); > > The NULL in this constant got me thinking. Are we getting things correct w.r.t. null termination? For example, the buffer in String isn't null terminated. If V8 is trying to write a null terminator, the lengths will be off by one. I guess that would show up in tests if we were goofing that up. PRESERVE_ASCII_NULL is a bug fix for some bizarre legacy thing in v8 where '\0' was swapped out with ' '. As far as null termination goes, v8 doesn't need it and doesn't add null terminators, just like webcore >> Source/WebCore/bindings/v8/V8StringResource.cpp:125 >> + } > > This code looks largely duplicated from V8PerIsolateData.cpp. Is there no way to share this logic? > > I also don't understand why we need this tricky static_cast logic (same question as above) I was going to share the logic, but this particular block gets hit literally billions of times in certain dromeao tests, so i duplicated the code. Otherwise there would need to be an additional branch here. >> Source/WebCore/bindings/v8/V8StringResource.cpp:129 >> + if (UNLIKELY(!length)) > > Did you measure this UNLIKELY as having a performance benefit? It's most likely a no-op. I didn't measure the performance impact, but because this code gets hit hard in the benchmarks (less so than the above block), i added it. I figured it might swap the branch contents to put everything after the branch in starting at the next instruction, but i didn't bother to check. >> Source/WebCore/bindings/v8/V8ValueCache.h:151 >> + return static_cast<WebCoreStringResource16*>(v8String->GetExternalStringResource()); > > Should we add an ASSERT about the encoding of v8String? this might no longer be used. if so i'll drop it, but the original intention of the call was to check whether the resource existed, as v8 has no cheap way of checking so the assert would be incorrect. (In reply to comment #33) > Some of those perf results show large swings in "bytes". Is that expected? I'll double check once the v8 fix is in and this patch can go out, but those number were largely random in my original tests. Ah, I think I didn't understand the class hierarchy quite correctly. Thanks. This patch seems close to ready to be landed once we roll in a new version of V8. Comment on attachment 169406 [details]
Patch
I've adjusted the flags to more closely match my understanding of the status of this patch.
Created attachment 171452 [details]
Patch
Comment on attachment 171452 [details]
Patch
The necessary v8 fix has been committed.
Comment on attachment 171452 [details] Patch Clearing flags on attachment: 171452 Committed r132913: <http://trac.webkit.org/changeset/132913> All reviewed patches have been landed. Closing bug. Reopening, this was rolled out in https://bugs.webkit.org/show_bug.cgi?id=100872. Created attachment 174676 [details]
Patch
(In reply to comment #43) > Created an attachment (id=174676) [details] > Patch I believe I've addressed the performance issues that caused the rollback of the last patch. See bug 100872 for more details. Comment on attachment 174676 [details]
Patch
let's give it another try
Comment on attachment 174676 [details] Patch Clearing flags on attachment: 174676 Committed r134950: <http://trac.webkit.org/changeset/134950> All reviewed patches have been landed. Closing bug. I'm very happy to see this patch land again. :) Yes, thank you Dan! 3% memory improvement: http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/morejs/report.html?rev=168337&graph=ws_single_peak_r&trace=ws_spk_r&history=30 No performance regressions yet. Fingers crossed! Not sure what to make of this data. But it looks like there's a regression on Mac. Maybe there some conversion we're doing only on Mac with this patch? 17% improvement on DOMDivWalk on Windows and Linux: http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dom_perf/report.html?rev=168452&graph=DOMDivWalk&trace=score&history=50 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dom_perf/report.html?rev=168452&graph=Get%20Elements&trace=score&history=50 12% improvement on jslib_attr_jquery_jQuery___addClass on Mac 10.6: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibattrjquery/report.html?rev=168452&graph=jslib_attr_jquery_jQuery___addClass&trace=score&history=50 9% regression on DOMDivWalk on Mac 10.6: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?rev=168452&graph=DOMDivWalk&trace=score&history=50 8% regression on GetElements on Mac 10.6: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?rev=168452&graph=Get%20Elements&trace=score&history=50 Mac is the only platform on which we don't use tcmalloc (I believe by mistake): http://code.google.com/p/chromium/issues/detail?id=158645 (In reply to comment #51) > Not sure what to make of this data. But it looks like there's a regression on Mac. Maybe there some conversion we're doing only on Mac with this patch? I only tested on linux, but I can look further into why this is happening. Comment on attachment 174676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174676&action=review > Source/WebCore/bindings/v8/V8ValueCache.h:86 > + v8::V8::AdjustAmountOfExternalAllocatedMemory(memoryConsumption(string)); Here and elsewhere: memoryConsumption()? I believe you can get that from WTF::String.sizeInBytes(), right? Nice work! This patch produced a ~2.5% improvement on two SVG tests on linux: http://webkit-perf.appspot.com/graph.html#tests=[[10927939,2001,173262],[11590529,2001,173262]]&sel=1353031234357.7446,1353130502442.851&displayrange=30&datatype=running |