Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16158008:
Mobile Gmail should use 1.3 MB less memory (Closed)
|
Created:
7 years, 6 months ago by abarth-chromium Modified:
7 years, 6 months ago CC:
blink-reviews, jsbell+bindings_chromium.org, eae+blinkwatch, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionMobile Gmail should use 1.3 MB less memory
The vast majority of the remaining 16 bit strings in Mobile Gmail are coming
from our v8::String to WTF::String conversion. Before this CL, we were relying
upon V8 to detect 8 bit strings. This CL makes it so we check whether the
resulting WTF::String fits in 8 bits when we create it from the v8::String.
(Note that we also externalize the v8::String when we convert it to a
WTF::String, so we shrink the V8 representation of the string at the same
time.)
The STRING_STATS for this change are below. My test environment isn't 100%
reproducable, so the data isn't completely consistent. For example, there are
283,552 more toal characters in the "before" measurement than in the "after"
measurement, which biases the results slightly. However, even if those missing
strings were all 16 bits wide, this CL would still be a >1 MB memory win.
= Mobile Gmail =
== Before ==
String stats for process id 23182:
5311 (98.99%) 8 bit 1877856 chars 1877856 bytes avg length 353.6
54 ( 1.01%) 16 bit 1839813 chars 3679626 bytes avg length 34070.6
8 ( 0.15%) upconverted 1138 chars 2276 bytes avg length 142.2
5365 Total 3717669 chars 5559758 bytes avg length 692.9
Total savings 1876718 bytes (25.24%)
== After ==
String stats for process id 23382:
5307 (99.31%) 8 bit 2687571 chars 2687571 bytes avg length 506.4
38 ( 0.71%) 16 bit 746572 chars 1493144 bytes avg length 19646.6
6 ( 0.11%) upconverted 1112 chars 2224 bytes avg length 185.3
5344 Total 3434143 chars 4182939 bytes avg length 642.6
Total savings 2686459 bytes (39.11%)
NOTRY=true
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151994
Patch Set 1 #
Total comments: 8
Patch Set 2 : Update to new V8 API #Messages
Total messages: 23 (0 generated)
Sorry the stats in the commit message don't show up that nicely. I've put them on pastebin as well: http://pastebin.com/pbB5s0nd Mobile Facebook also shows a significant decrease in 16 bit strings, upwards of 500 kB.
One thing that I find interesting is that the total number of string affected is small---they're just quite large.
Very interesting! dcarney should take a look. > Before this CL, we were relying upon V8 to detect 8 bit strings. This CL makes it so we check whether the resulting WTF::String fits in 8 bits when we create it from the v8::String Why can't V8 detect it perfectly? I'm curious about why Blink can make a better judgement. https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... File Source/bindings/v8/V8StringResource.cpp (right): https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... Source/bindings/v8/V8StringResource.cpp:158: return String(""); // FIXME: Should return emptyString(); Nit: Why is it hard to fix it now? I'm just curious.
https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... File Source/bindings/v8/V8StringResource.cpp (right): https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... Source/bindings/v8/V8StringResource.cpp:158: return String(""); // FIXME: Should return emptyString(); On 2013/06/06 05:22:11, haraken wrote: > Nit: Why is it hard to fix it now? I'm just curious. I'd prefer to fix it in a second CL. It should be fine to fix, but I didn't want to add risk to this CL.
On 2013/06/06 05:22:11, haraken wrote: > > Before this CL, we were relying > upon V8 to detect 8 bit strings. This CL makes it so we check whether the > resulting WTF::String fits in 8 bits when we create it from the v8::String > > Why can't V8 detect it perfectly? I'm curious about why Blink can make a better > judgement. I'm not sure. It's likely there are improvements we can make on the V8 side as well to not bloat 8 bit strings to 16 bits. I've been burning down the 16 bit bloat on the Blink side. I suspect it would be valuable for someone knowledgeable about V8 internals to do the same on the V8 side.
lgtm Seems like this is papering over v8 bugs... but I accept the memory win! :) https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... File Source/bindings/v8/V8StringResource.cpp (right): https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... Source/bindings/v8/V8StringResource.cpp:86: if (result.containsOnlyLatin1()) Every time we hit this it seems this is an error from v8? Should we have a FIXME here?
> > Why can't V8 detect it perfectly? I'm curious about why Blink can make a > better > > judgement. > > I'm not sure. It's likely there are improvements we can make on the V8 side as > well to not bloat 8 bit strings to 16 bits. I've been burning down the 16 bit > bloat on the Blink side. I suspect it would be valuable for someone > knowledgeable about V8 internals to do the same on the V8 side. Makes sense. LGTM2.
https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... File Source/bindings/v8/V8StringResource.cpp (right): https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... Source/bindings/v8/V8StringResource.cpp:86: if (result.containsOnlyLatin1()) On 2013/06/06 05:49:08, eseidel wrote: > Every time we hit this it seems this is an error from v8? Should we have a > FIXME here? I wouldn't say there is an error in V8. It's fine to represent Latin-1 strings using 16 bits per character. It's just not as memory-efficient as using 8 bits per character. As an example, suppose you have a long string with many non-Latin-1 characters. If you take a substring, you'll probably end up with a 16 bit wide string both in V8 and with WTFString. With WTFString, you can even end up sharing the underlying string buffer, so it's actually more memory-efficient to use a 16 bit representation. That said, in the common case, you'd hope that entirely Latin-1 strings would only use 8 bits per character. I agree that this CL suggests that we should do some more investigation on the V8 side to understand why we're getting all these 16 bit strings here. I'm secretly hoping dcarney is interested in doing that investigation. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abarth@chromium.org/16158008/1
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_layout_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://chromiumcodereview.appspot.com/16158008/diff/1/Source/bindings/v8/V8S... File Source/bindings/v8/V8StringResource.cpp (right): https://chromiumcodereview.appspot.com/16158008/diff/1/Source/bindings/v8/V8S... Source/bindings/v8/V8StringResource.cpp:86: if (result.containsOnlyLatin1()) On 2013/06/06 05:49:08, eseidel wrote: > Every time we hit this it seems this is an error from v8? Should we have a > FIXME here? there are plenty on cases where the cost of checking 8 bitness is too high in v8 to do, but the root cause is often the concatenation of many small strings, one of which fulfills this condition https://chromiumcodereview.appspot.com/16158008/diff/1/Source/bindings/v8/V8S... Source/bindings/v8/V8StringResource.cpp:97: UChar inlineBuffer[inlineBufferSize]; are small strings not targeted here for a reason? https://chromiumcodereview.appspot.com/16158008/diff/1/Source/bindings/v8/V8S... Source/bindings/v8/V8StringResource.cpp:104: if (result.containsOnlyLatin1()) { maybe this check should be moved to the atomicstring table, so that all 8 bit strings in the atomic string table are actually 8 bit and not 16? last time i investigated, there were tons of such strings in the atomic string tables
actually, the more i think about this, the more I think this should be done in v8. it's more efficient to pass v8 a flag to check for 8 bitness in the write call. and write the latin1 version into the buffer directly. that way v8 can flag the string internally as 8 bit as well, which helps later calls.
https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... File Source/bindings/v8/V8StringResource.cpp (right): https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringRe... Source/bindings/v8/V8StringResource.cpp:97: UChar inlineBuffer[inlineBufferSize]; On 2013/06/06 06:45:20, dcarney wrote: > are small strings not targeted here for a reason? Yes. The AtomicString(inlineBuffer, length) constructor already checks whether the buffer is 8 bit.
On 2013/06/06 06:50:04, dcarney wrote: > actually, the more i think about this, the more I think this should be done in > v8. it's more efficient to pass v8 a flag to check for 8 bitness in the write > call. and write the latin1 version into the buffer directly. Yes, that would be lovely! It seems really silly to copy out the 16 bit string just to copy again into an 8 bit buffer. > that way v8 can flag the string internally as 8 bit as well, which helps later calls. My understanding is that these functions are called in the context of v8StringToWebCoreString, which calls MakeExternal on the v8::String, so we don't keep two copies of the string buffer. What's the best way to structure this API? Maybe we should replace the v8String->IsOneByte() call with a call that scans the string to see how wide an external buffer we need?
> What's the best way to structure this API? Maybe we should replace the > v8String->IsOneByte() call with a call that scans the string to see how wide an > external buffer we need? yes, that's what I was thinking - ContainsOnlyOneByteCharacters() or something along those lines. Then this CL resolves to a one line change. I'll see if I can get it in today.
On 2013/06/06 07:25:53, dcarney wrote: > yes, that's what I was thinking - ContainsOnlyOneByteCharacters() or something > along those lines. Then this CL resolves to a one line change. I'll see if I > can get it in today. That would be wonderful. Thanks Dan!!
On 2013/06/06 07:43:23, abarth wrote: > On 2013/06/06 07:25:53, dcarney wrote: > > yes, that's what I was thinking - ContainsOnlyOneByteCharacters() or something > > along those lines. Then this CL resolves to a one line change. I'll see if I > > can get it in today. > > That would be wonderful. Thanks Dan!! landed in bleeding_edge https://codereview.chromium.org/16530003/ i don't know when the next v8 roll will be
I've rebased this CL on top of the new V8 API. I'll land it once the new API is visible. Seems to work great!
On 2013/06/06 19:03:39, abarth wrote: > I've rebased this CL on top of the new V8 API. I'll land it once the new API is > visible. Seems to work great! okay. it shouldn't be the case that dromeao hits any of these strings, but if there are any perf regressions, there's a follow up patch https://codereview.chromium.org/16147004/ which should be a bit faster for the case where long strings need to be read
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abarth@chromium.org/16158008/16001
Retried try job too often on win_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abarth@chromium.org/16158008/16001
Message was sent while issue was closed.
Change committed as 151994 |
