Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(28)

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
Reviewers:
haraken, dcarney, eseidel
CC:
blink-reviews, jsbell+bindings_chromium.org, eae+blinkwatch, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin
Visibility:
Public.

Description

Mobile 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -12 lines) Patch
M Source/bindings/v8/V8StringResource.cpp View 1 5 chunks +3 lines, -12 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
abarth-chromium
Sorry the stats in the commit message don't show up that nicely. I've put them ...
7 years, 6 months ago (2013-06-06 01:33:51 UTC) #1
abarth-chromium
One thing that I find interesting is that the total number of string affected is ...
7 years, 6 months ago (2013-06-06 01:35:17 UTC) #2
haraken
Very interesting! dcarney should take a look. > Before this CL, we were relying upon ...
7 years, 6 months ago (2013-06-06 05:22:11 UTC) #3
abarth-chromium
https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringResource.cpp File Source/bindings/v8/V8StringResource.cpp (right): https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringResource.cpp#newcode158 Source/bindings/v8/V8StringResource.cpp:158: return String(""); // FIXME: Should return emptyString(); On 2013/06/06 ...
7 years, 6 months ago (2013-06-06 05:24:02 UTC) #4
abarth-chromium
On 2013/06/06 05:22:11, haraken wrote: > > Before this CL, we were relying > upon ...
7 years, 6 months ago (2013-06-06 05:25:45 UTC) #5
eseidel
lgtm Seems like this is papering over v8 bugs... but I accept the memory win! ...
7 years, 6 months ago (2013-06-06 05:49:08 UTC) #6
haraken
> > Why can't V8 detect it perfectly? I'm curious about why Blink can make ...
7 years, 6 months ago (2013-06-06 05:52:57 UTC) #7
abarth-chromium
https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringResource.cpp File Source/bindings/v8/V8StringResource.cpp (right): https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringResource.cpp#newcode86 Source/bindings/v8/V8StringResource.cpp:86: if (result.containsOnlyLatin1()) On 2013/06/06 05:49:08, eseidel wrote: > Every ...
7 years, 6 months ago (2013-06-06 06:01:54 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abarth@chromium.org/16158008/1
7 years, 6 months ago (2013-06-06 06:02:10 UTC) #9
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 6 months ago (2013-06-06 06:29:39 UTC) #10
dcarney
https://chromiumcodereview.appspot.com/16158008/diff/1/Source/bindings/v8/V8StringResource.cpp File Source/bindings/v8/V8StringResource.cpp (right): https://chromiumcodereview.appspot.com/16158008/diff/1/Source/bindings/v8/V8StringResource.cpp#newcode86 Source/bindings/v8/V8StringResource.cpp:86: if (result.containsOnlyLatin1()) On 2013/06/06 05:49:08, eseidel wrote: > Every ...
7 years, 6 months ago (2013-06-06 06:45:20 UTC) #11
dcarney
actually, the more i think about this, the more I think this should be done ...
7 years, 6 months ago (2013-06-06 06:50:04 UTC) #12
abarth-chromium
https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringResource.cpp File Source/bindings/v8/V8StringResource.cpp (right): https://codereview.chromium.org/16158008/diff/1/Source/bindings/v8/V8StringResource.cpp#newcode97 Source/bindings/v8/V8StringResource.cpp:97: UChar inlineBuffer[inlineBufferSize]; On 2013/06/06 06:45:20, dcarney wrote: > are ...
7 years, 6 months ago (2013-06-06 06:55:26 UTC) #13
abarth-chromium
On 2013/06/06 06:50:04, dcarney wrote: > actually, the more i think about this, the more ...
7 years, 6 months ago (2013-06-06 06:59:50 UTC) #14
dcarney
> What's the best way to structure this API? Maybe we should replace the > ...
7 years, 6 months ago (2013-06-06 07:25:53 UTC) #15
abarth-chromium
On 2013/06/06 07:25:53, dcarney wrote: > yes, that's what I was thinking - ContainsOnlyOneByteCharacters() or ...
7 years, 6 months ago (2013-06-06 07:43:23 UTC) #16
drcarney
On 2013/06/06 07:43:23, abarth wrote: > On 2013/06/06 07:25:53, dcarney wrote: > > yes, that's ...
7 years, 6 months ago (2013-06-06 13:18:46 UTC) #17
abarth-chromium
I've rebased this CL on top of the new V8 API. I'll land it once ...
7 years, 6 months ago (2013-06-06 19:03:39 UTC) #18
dcarney
On 2013/06/06 19:03:39, abarth wrote: > I've rebased this CL on top of the new ...
7 years, 6 months ago (2013-06-06 20:27:01 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abarth@chromium.org/16158008/16001
7 years, 6 months ago (2013-06-06 23:03:04 UTC) #20
commit-bot: I haz the power
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_rel&number=9116
7 years, 6 months ago (2013-06-07 02:29:43 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abarth@chromium.org/16158008/16001
7 years, 6 months ago (2013-06-07 04:16:45 UTC) #22
commit-bot: I haz the power
7 years, 6 months ago (2013-06-07 04:16:56 UTC) #23
Message was sent while issue was closed.
Change committed as 151994

Powered by Google App Engine
This is Rietveld 408576698