update_engine: Only split operation if it has multiple dst extents.
FragmentOperations() will split operations so that every operation will
have only one dst extent, but if the operation already meet this
requirement, it should be a no-op and we shouldn't waste time recompressing
it. This can save a lot of time if there's huge replace operation that
only has one dst extent.
As a side effect of not splitting those operations, if they are SOURCE_COPY
operations, they will not have src_length and dst_length set anymore,
because those were set in SplitSourceCopy() before this change, but these
lengths are never actually used in delta performer since the very first
implementation of PerformSourceCopyOperation():
https://chromium-review.googlesource.com/c/263747/
So it's safe to remove it from the manifest. We might as well just remove
src_length and dst_length in all SOURCE_COPY operations (even for those
are splitted) to reduce payload size a little bit.
BUG=b:62470452
TEST=generated a payload, all operations are still the same except
SOURCE_COPY don't have src_length and dst_length any more
Change-Id: I019ca9056479165e76cc25d7a4ba453088398928
(cherry picked from commit 929461ab11769339108658f2022d068446ca90f7)
Reviewed-on: https://chromium-review.googlesource.com/594909
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Sen Jiang <senj@chromium.org>
diff --git a/payload_generator/ab_generator.cc b/payload_generator/ab_generator.cc
index efb8ccf..3b0d012 100644
--- a/payload_generator/ab_generator.cc
+++ b/payload_generator/ab_generator.cc
@@ -88,14 +88,19 @@
BlobFileWriter* blob_file) {
vector<AnnotatedOperation> fragmented_aops;
for (const AnnotatedOperation& aop : *aops) {
- if (aop.op.type() == InstallOperation::SOURCE_COPY) {
- TEST_AND_RETURN_FALSE(SplitSourceCopy(aop, &fragmented_aops));
- } else if (IsAReplaceOperation(aop.op.type())) {
- TEST_AND_RETURN_FALSE(SplitAReplaceOp(
- version, aop, target_part_path, &fragmented_aops, blob_file));
- } else {
- fragmented_aops.push_back(aop);
+ // Only do split if the operation has more than one dst extents.
+ if (aop.op.dst_extents_size() > 1) {
+ if (aop.op.type() == InstallOperation::SOURCE_COPY) {
+ TEST_AND_RETURN_FALSE(SplitSourceCopy(aop, &fragmented_aops));
+ continue;
+ }
+ if (IsAReplaceOperation(aop.op.type())) {
+ TEST_AND_RETURN_FALSE(SplitAReplaceOp(
+ version, aop, target_part_path, &fragmented_aops, blob_file));
+ continue;
+ }
}
+ fragmented_aops.push_back(aop);
}
*aops = std::move(fragmented_aops);
return true;
@@ -139,8 +144,6 @@
// Fix up our new operation and add it to the results.
new_op.set_type(InstallOperation::SOURCE_COPY);
*(new_op.add_dst_extents()) = dst_ext;
- new_op.set_src_length(dst_ext.num_blocks() * kBlockSize);
- new_op.set_dst_length(dst_ext.num_blocks() * kBlockSize);
AnnotatedOperation new_aop;
new_aop.op = new_op;
diff --git a/payload_generator/ab_generator_unittest.cc b/payload_generator/ab_generator_unittest.cc
index 3fd2323..ab4b164 100644
--- a/payload_generator/ab_generator_unittest.cc
+++ b/payload_generator/ab_generator_unittest.cc
@@ -354,11 +354,11 @@
EXPECT_EQ("SplitSourceCopyTestOp:0", result_ops[0].name);
InstallOperation first_op = result_ops[0].op;
EXPECT_EQ(InstallOperation::SOURCE_COPY, first_op.type());
- EXPECT_EQ(kBlockSize * 2, first_op.src_length());
+ EXPECT_FALSE(first_op.has_src_length());
EXPECT_EQ(1, first_op.src_extents().size());
EXPECT_EQ(2U, first_op.src_extents(0).start_block());
EXPECT_EQ(2U, first_op.src_extents(0).num_blocks());
- EXPECT_EQ(kBlockSize * 2, first_op.dst_length());
+ EXPECT_FALSE(first_op.has_dst_length());
EXPECT_EQ(1, first_op.dst_extents().size());
EXPECT_EQ(10U, first_op.dst_extents(0).start_block());
EXPECT_EQ(2U, first_op.dst_extents(0).num_blocks());
@@ -366,7 +366,7 @@
EXPECT_EQ("SplitSourceCopyTestOp:1", result_ops[1].name);
InstallOperation second_op = result_ops[1].op;
EXPECT_EQ(InstallOperation::SOURCE_COPY, second_op.type());
- EXPECT_EQ(kBlockSize * 3, second_op.src_length());
+ EXPECT_FALSE(second_op.has_src_length());
EXPECT_EQ(3, second_op.src_extents().size());
EXPECT_EQ(4U, second_op.src_extents(0).start_block());
EXPECT_EQ(1U, second_op.src_extents(0).num_blocks());
@@ -374,7 +374,7 @@
EXPECT_EQ(1U, second_op.src_extents(1).num_blocks());
EXPECT_EQ(8U, second_op.src_extents(2).start_block());
EXPECT_EQ(1U, second_op.src_extents(2).num_blocks());
- EXPECT_EQ(kBlockSize * 3, second_op.dst_length());
+ EXPECT_FALSE(second_op.has_dst_length());
EXPECT_EQ(1, second_op.dst_extents().size());
EXPECT_EQ(14U, second_op.dst_extents(0).start_block());
EXPECT_EQ(3U, second_op.dst_extents(0).num_blocks());
@@ -382,11 +382,11 @@
EXPECT_EQ("SplitSourceCopyTestOp:2", result_ops[2].name);
InstallOperation third_op = result_ops[2].op;
EXPECT_EQ(InstallOperation::SOURCE_COPY, third_op.type());
- EXPECT_EQ(kBlockSize * 3, third_op.src_length());
+ EXPECT_FALSE(third_op.has_src_length());
EXPECT_EQ(1, third_op.src_extents().size());
EXPECT_EQ(9U, third_op.src_extents(0).start_block());
EXPECT_EQ(3U, third_op.src_extents(0).num_blocks());
- EXPECT_EQ(kBlockSize * 3, third_op.dst_length());
+ EXPECT_FALSE(third_op.has_dst_length());
EXPECT_EQ(1, third_op.dst_extents().size());
EXPECT_EQ(18U, third_op.dst_extents(0).start_block());
EXPECT_EQ(3U, third_op.dst_extents(0).num_blocks());
@@ -445,8 +445,6 @@
vector<AnnotatedOperation> aops;
InstallOperation first_op;
first_op.set_type(InstallOperation::SOURCE_COPY);
- first_op.set_src_length(kBlockSize);
- first_op.set_dst_length(kBlockSize);
*(first_op.add_src_extents()) = ExtentForRange(1, 1);
*(first_op.add_dst_extents()) = ExtentForRange(6, 1);
AnnotatedOperation first_aop;
@@ -456,8 +454,6 @@
InstallOperation second_op;
second_op.set_type(InstallOperation::SOURCE_COPY);
- second_op.set_src_length(3 * kBlockSize);
- second_op.set_dst_length(3 * kBlockSize);
*(second_op.add_src_extents()) = ExtentForRange(2, 2);
*(second_op.add_src_extents()) = ExtentForRange(8, 2);
*(second_op.add_dst_extents()) = ExtentForRange(7, 3);
@@ -469,8 +465,6 @@
InstallOperation third_op;
third_op.set_type(InstallOperation::SOURCE_COPY);
- third_op.set_src_length(kBlockSize);
- third_op.set_dst_length(kBlockSize);
*(third_op.add_src_extents()) = ExtentForRange(11, 1);
*(third_op.add_dst_extents()) = ExtentForRange(12, 1);
AnnotatedOperation third_aop;
@@ -486,12 +480,12 @@
EXPECT_EQ(1U, aops.size());
InstallOperation first_result_op = aops[0].op;
EXPECT_EQ(InstallOperation::SOURCE_COPY, first_result_op.type());
- EXPECT_EQ(kBlockSize * 5, first_result_op.src_length());
+ EXPECT_FALSE(first_result_op.has_src_length());
EXPECT_EQ(3, first_result_op.src_extents().size());
EXPECT_TRUE(ExtentEquals(first_result_op.src_extents(0), 1, 3));
EXPECT_TRUE(ExtentEquals(first_result_op.src_extents(1), 8, 2));
EXPECT_TRUE(ExtentEquals(first_result_op.src_extents(2), 11, 1));
- EXPECT_EQ(kBlockSize * 5, first_result_op.dst_length());
+ EXPECT_FALSE(first_result_op.has_dst_length());
EXPECT_EQ(2, first_result_op.dst_extents().size());
EXPECT_TRUE(ExtentEquals(first_result_op.dst_extents(0), 6, 4));
EXPECT_TRUE(ExtentEquals(first_result_op.dst_extents(1), 11, 2));