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

Issue 2969123002: Add deduplication logic to .pak files (Closed)

Created:
3 years, 5 months ago by agrieve
Modified:
3 years, 5 months ago
Reviewers:
flackr, sadrul
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add deduplication logic to .pak files Now, when multiple entries contain the same content, multiple table-of-contents entries will be created, but only one data region. As of now, en-US.pak has ~3200 entries, and 350 of them are duplicates. For MonochromePublic.apk, shrinks uncompressed .pak size by ~130kb, and compressed .pak size by 32kb. BUG=738566 Review-Url: https://codereview.chromium.org/2969123002 Cr-Commit-Position: refs/heads/master@{#488215} Committed: https://chromium.googlesource.com/chromium/src/+/ade347f539a378fb37500f6d17e8835edc1d8ec0

Patch Set 1 #

Patch Set 2 : Add deduplication logic to .pak files #

Patch Set 3 : self review #

Patch Set 4 : Convert to alias table #

Patch Set 5 : update pak_util.py #

Patch Set 6 : fix resource_sizes computation #

Total comments: 31

Patch Set 7 : review comments #

Patch Set 8 : Bigger header, write a v5 #

Total comments: 2

Patch Set 9 : review comments #

Total comments: 2

Patch Set 10 : sizeof() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+396 lines, -213 lines) Patch
M build/android/resource_sizes.py View 1 2 3 4 5 6 7 1 chunk +9 lines, -4 lines 0 comments Download
M tools/grit/grit/format/data_pack.py View 1 2 3 4 5 6 7 3 chunks +80 lines, -34 lines 0 comments Download
M tools/grit/grit/format/data_pack_unittest.py View 1 2 3 4 5 6 7 3 chunks +42 lines, -7 lines 0 comments Download
M tools/grit/pak_util.py View 1 2 3 4 5 6 7 2 chunks +8 lines, -2 lines 0 comments Download
M ui/base/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M ui/base/resource/data_pack.h View 1 2 3 4 2 chunks +6 lines, -1 line 0 comments Download
M ui/base/resource/data_pack.cc View 1 2 3 4 5 6 7 8 9 8 chunks +130 lines, -96 lines 0 comments Download
A ui/base/resource/data_pack_literal.h View 1 1 chunk +23 lines, -0 lines 0 comments Download
M ui/base/resource/data_pack_literal.cc View 1 2 3 4 5 6 7 1 chunk +57 lines, -43 lines 0 comments Download
M ui/base/resource/data_pack_unittest.cc View 1 8 chunks +37 lines, -16 lines 0 comments Download
M ui/base/resource/resource_bundle_unittest.cc View 1 3 chunks +3 lines, -10 lines 0 comments Download

Messages

Total messages: 40 (18 generated)
agrieve
📟
3 years, 5 months ago (2017-07-05 18:07:27 UTC) #2
flackr
I have an alternate proposal to having a bit mean an ID instead of a ...
3 years, 5 months ago (2017-07-05 20:24:03 UTC) #3
agrieve
On 2017/07/05 20:24:03, flackr wrote: > I have an alternate proposal to having a bit ...
3 years, 5 months ago (2017-07-05 20:37:05 UTC) #4
flackr
On 2017/07/05 20:37:05, agrieve wrote: > On 2017/07/05 20:24:03, flackr wrote: > > I have ...
3 years, 5 months ago (2017-07-05 20:41:25 UTC) #5
flackr
On 2017/07/05 20:41:25, flackr wrote: > On 2017/07/05 20:37:05, agrieve wrote: > > On 2017/07/05 ...
3 years, 5 months ago (2017-07-05 20:44:28 UTC) #6
flackr
On 2017/07/05 20:44:28, flackr wrote: > On 2017/07/05 20:41:25, flackr wrote: > > On 2017/07/05 ...
3 years, 5 months ago (2017-07-05 20:51:25 UTC) #7
agrieve
On 2017/07/05 20:51:25, flackr wrote: > On 2017/07/05 20:44:28, flackr wrote: > > On 2017/07/05 ...
3 years, 5 months ago (2017-07-06 03:50:04 UTC) #8
flackr
On 2017/07/06 03:50:04, agrieve wrote: > On 2017/07/05 20:51:25, flackr wrote: > > On 2017/07/05 ...
3 years, 5 months ago (2017-07-06 13:35:19 UTC) #9
agrieve
On 2017/07/06 13:35:19, flackr wrote: > On 2017/07/06 03:50:04, agrieve wrote: > > On 2017/07/05 ...
3 years, 5 months ago (2017-07-06 17:56:56 UTC) #11
flackr
https://codereview.chromium.org/2969123002/diff/100001/tools/grit/grit/format/data_pack.py File tools/grit/grit/format/data_pack.py (right): https://codereview.chromium.org/2969123002/diff/100001/tools/grit/grit/format/data_pack.py#newcode80 tools/grit/grit/format/data_pack.py:80: prev_offset = entry_at_index(resource_count)[1] It's a little strange to not ...
3 years, 5 months ago (2017-07-07 18:54:13 UTC) #16
agrieve
One comment to resolve still, but heading off on vacation. Sadrul - ptal. https://codereview.chromium.org/2969123002/diff/100001/tools/grit/grit/format/data_pack.py File ...
3 years, 5 months ago (2017-07-07 20:47:09 UTC) #17
flackr
https://codereview.chromium.org/2969123002/diff/100001/tools/grit/grit/format/data_pack.py File tools/grit/grit/format/data_pack.py (right): https://codereview.chromium.org/2969123002/diff/100001/tools/grit/grit/format/data_pack.py#newcode114 tools/grit/grit/format/data_pack.py:114: # Note: 2nd and 4th byte are always 0 ...
3 years, 5 months ago (2017-07-10 14:07:50 UTC) #18
agrieve
https://codereview.chromium.org/2969123002/diff/100001/tools/grit/grit/format/data_pack.py File tools/grit/grit/format/data_pack.py (right): https://codereview.chromium.org/2969123002/diff/100001/tools/grit/grit/format/data_pack.py#newcode114 tools/grit/grit/format/data_pack.py:114: # Note: 2nd and 4th byte are always 0 ...
3 years, 5 months ago (2017-07-18 19:30:29 UTC) #19
flackr
https://codereview.chromium.org/2969123002/diff/100001/ui/base/resource/data_pack.cc File ui/base/resource/data_pack.cc (right): https://codereview.chromium.org/2969123002/diff/100001/ui/base/resource/data_pack.cc#newcode207 ui/base/resource/data_pack.cc:207: if (data_source->GetLength() < kHeaderLengthV4) { On 2017/07/07 20:47:09, agrieve ...
3 years, 5 months ago (2017-07-18 20:21:23 UTC) #20
agrieve
https://codereview.chromium.org/2969123002/diff/100001/ui/base/resource/data_pack.cc File ui/base/resource/data_pack.cc (right): https://codereview.chromium.org/2969123002/diff/100001/ui/base/resource/data_pack.cc#newcode207 ui/base/resource/data_pack.cc:207: if (data_source->GetLength() < kHeaderLengthV4) { On 2017/07/18 20:21:23, flackr ...
3 years, 5 months ago (2017-07-19 02:34:05 UTC) #21
flackr
LGTM with nit, thanks for bearing with me on the concerns. https://codereview.chromium.org/2969123002/diff/160001/ui/base/resource/data_pack.cc File ui/base/resource/data_pack.cc (right): ...
3 years, 5 months ago (2017-07-19 03:05:04 UTC) #22
agrieve
And thanks for putting thought into the reviews! Sadrul - ptal. https://codereview.chromium.org/2969123002/diff/160001/ui/base/resource/data_pack.cc File ui/base/resource/data_pack.cc (right): ...
3 years, 5 months ago (2017-07-19 14:59:01 UTC) #23
sadrul
lgtm
3 years, 5 months ago (2017-07-20 02:28:07 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2969123002/180001
3 years, 5 months ago (2017-07-20 12:42:28 UTC) #35
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/ade347f539a378fb37500f6d17e8835edc1d8ec0
3 years, 5 months ago (2017-07-20 12:48:33 UTC) #38
achuithb
On 2017/07/20 12:48:33, commit-bot: I haz the power wrote: > Committed patchset #10 (id:180001) as ...
3 years, 5 months ago (2017-07-20 23:53:20 UTC) #39
achuithb
3 years, 5 months ago (2017-07-20 23:55:40 UTC) #40
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2989443002/ by achuith@chromium.org.

The reason for reverting is:
https://bugs.chromium.org/p/chromium/issues/detail?id=747171.

Powered by Google App Engine
This is Rietveld 408576698