AU: Don't send no-op operations in the delta payload.
This reduces the payload a bit and also speeds up updates on the client by
reducing the I/O.
This CL also removes a somewhat broken check for written blocks. It seems that
the intent of the check was to make sure each block is updated with some data
but since some file data blocks may be used as scratch the check is somewhat
meaningless. The CL removes the check because some blocks may never be written
if they were a part of a no-op operation.
BUG=7543
TEST=unit tests, generated a no-op delta an installed on the device
Change-Id: I31f388651a45d10dd931389a6c80ac18cb10f466
Review URL: http://codereview.chromium.org/3785008
diff --git a/delta_diff_generator.cc b/delta_diff_generator.cc
index 43192ee..b447f11 100644
--- a/delta_diff_generator.cc
+++ b/delta_diff_generator.cc
@@ -47,6 +47,8 @@
namespace chromeos_update_engine {
typedef DeltaDiffGenerator::Block Block;
+typedef map<const DeltaArchiveManifest_InstallOperation*,
+ const string*> OperationNameMap;
namespace {
const size_t kBlockSize = 4096; // bytes
@@ -376,24 +378,37 @@
return true;
}
-// Adds each operation from the graph to the manifest in the order
-// specified by 'order'.
+// Adds each operation from |graph| to |out_manifest| in the order specified by
+// |order| while building |out_op_name_map| with operation to name
+// mappings. Adds all |kernel_ops| to |out_manifest|. Filters out no-op
+// operations.
void InstallOperationsToManifest(
const Graph& graph,
const vector<Vertex::Index>& order,
const vector<DeltaArchiveManifest_InstallOperation>& kernel_ops,
- DeltaArchiveManifest* out_manifest) {
+ DeltaArchiveManifest* out_manifest,
+ OperationNameMap* out_op_name_map) {
for (vector<Vertex::Index>::const_iterator it = order.begin();
it != order.end(); ++it) {
+ const Vertex& vertex = graph[*it];
+ const DeltaArchiveManifest_InstallOperation& add_op = vertex.op;
+ if (DeltaDiffGenerator::IsNoopOperation(add_op)) {
+ continue;
+ }
DeltaArchiveManifest_InstallOperation* op =
out_manifest->add_install_operations();
- *op = graph[*it].op;
+ *op = add_op;
+ (*out_op_name_map)[op] = &vertex.file_name;
}
for (vector<DeltaArchiveManifest_InstallOperation>::const_iterator it =
kernel_ops.begin(); it != kernel_ops.end(); ++it) {
+ const DeltaArchiveManifest_InstallOperation& add_op = *it;
+ if (DeltaDiffGenerator::IsNoopOperation(add_op)) {
+ continue;
+ }
DeltaArchiveManifest_InstallOperation* op =
out_manifest->add_kernel_install_operations();
- *op = *it;
+ *op = add_op;
}
}
@@ -453,22 +468,20 @@
off_t size;
};
-void ReportPayloadUsage(const Graph& graph,
- const DeltaArchiveManifest& manifest,
- const int64_t manifest_metadata_size) {
+void ReportPayloadUsage(const DeltaArchiveManifest& manifest,
+ const int64_t manifest_metadata_size,
+ const OperationNameMap& op_name_map) {
vector<DeltaObject> objects;
off_t total_size = 0;
- // Graph nodes with information about file names.
- for (Vertex::Index node = 0; node < graph.size(); node++) {
- const Vertex& vertex = graph[node];
- if (!vertex.valid) {
- continue;
- }
- objects.push_back(DeltaObject(vertex.file_name,
- vertex.op.type(),
- vertex.op.data_length()));
- total_size += vertex.op.data_length();
+ // Rootfs install operations.
+ for (int i = 0; i < manifest.install_operations_size(); ++i) {
+ const DeltaArchiveManifest_InstallOperation& op =
+ manifest.install_operations(i);
+ objects.push_back(DeltaObject(*op_name_map.find(&op)->second,
+ op.type(),
+ op.data_length()));
+ total_size += op.data_length();
}
// Kernel install operations.
@@ -912,6 +925,14 @@
} // namespace {}
+// Returns true if |op| is a no-op operation that doesn't do any useful work
+// (e.g., a move operation that copies blocks onto themselves).
+bool DeltaDiffGenerator::IsNoopOperation(
+ const DeltaArchiveManifest_InstallOperation& op) {
+ return (op.type() == DeltaArchiveManifest_InstallOperation_Type_MOVE &&
+ ExpandExtents(op.src_extents()) == ExpandExtents(op.dst_extents()));
+}
+
bool DeltaDiffGenerator::AssignTempBlocks(
Graph* graph,
const string& new_root,
@@ -1370,9 +1391,13 @@
// Convert to protobuf Manifest object
DeltaArchiveManifest manifest;
+ OperationNameMap op_name_map;
CheckGraph(graph);
- InstallOperationsToManifest(graph, final_order, kernel_ops, &manifest);
-
+ InstallOperationsToManifest(graph,
+ final_order,
+ kernel_ops,
+ &manifest,
+ &op_name_map);
CheckGraph(graph);
manifest.set_block_size(kBlockSize);
@@ -1386,10 +1411,9 @@
temp_file_path,
ordered_blobs_path));
- // Check that install op blobs are in order and that all blocks are written.
+ // Check that install op blobs are in order.
uint64_t next_blob_offset = 0;
{
- vector<uint32_t> written_count(blocks.size(), 0);
for (int i = 0; i < (manifest.install_operations_size() +
manifest.kernel_install_operations_size()); i++) {
DeltaArchiveManifest_InstallOperation* op =
@@ -1397,14 +1421,6 @@
manifest.mutable_install_operations(i) :
manifest.mutable_kernel_install_operations(
i - manifest.install_operations_size());
- for (int j = 0; j < op->dst_extents_size(); j++) {
- const Extent& extent = op->dst_extents(j);
- for (uint64_t block = extent.start_block();
- block < (extent.start_block() + extent.num_blocks()); block++) {
- if (block < blocks.size())
- written_count[block]++;
- }
- }
if (op->has_data_offset()) {
if (op->data_offset() != next_blob_offset) {
LOG(FATAL) << "bad blob offset! " << op->data_offset() << " != "
@@ -1413,12 +1429,6 @@
next_blob_offset += op->data_length();
}
}
- // check all blocks written to
- for (vector<uint32_t>::size_type i = 0; i < written_count.size(); i++) {
- if (written_count[i] == 0) {
- LOG(FATAL) << "block " << i << " not written!";
- }
- }
}
// Signatures appear at the end of the blobs. Note the offset in the
@@ -1514,7 +1524,7 @@
int64_t manifest_metadata_size =
strlen(kDeltaMagic) + 2 * sizeof(uint64_t) + serialized_manifest.size();
- ReportPayloadUsage(graph, manifest, manifest_metadata_size);
+ ReportPayloadUsage(manifest, manifest_metadata_size, op_name_map);
LOG(INFO) << "All done. Successfully created delta file.";
return true;
diff --git a/delta_diff_generator.h b/delta_diff_generator.h
index de3be0d..9fbe0a5 100644
--- a/delta_diff_generator.h
+++ b/delta_diff_generator.h
@@ -211,6 +211,10 @@
std::vector<DeltaArchiveManifest_InstallOperation>* kernel_ops,
std::vector<Vertex::Index>* final_order);
+ // Returns true if |op| is a no-op operation that doesn't do any useful work
+ // (e.g., a move operation that copies blocks onto themselves).
+ static bool IsNoopOperation(const DeltaArchiveManifest_InstallOperation& op);
+
private:
// This should never be constructed
DISALLOW_IMPLICIT_CONSTRUCTORS(DeltaDiffGenerator);
diff --git a/delta_diff_generator_unittest.cc b/delta_diff_generator_unittest.cc
index d897a0a..ef4190f 100644
--- a/delta_diff_generator_unittest.cc
+++ b/delta_diff_generator_unittest.cc
@@ -588,4 +588,29 @@
}
}
+TEST_F(DeltaDiffGeneratorTest, IsNoopOperationTest) {
+ DeltaArchiveManifest_InstallOperation op;
+ op.set_type(DeltaArchiveManifest_InstallOperation_Type_REPLACE_BZ);
+ EXPECT_FALSE(DeltaDiffGenerator::IsNoopOperation(op));
+ op.set_type(DeltaArchiveManifest_InstallOperation_Type_MOVE);
+ EXPECT_TRUE(DeltaDiffGenerator::IsNoopOperation(op));
+ *(op.add_src_extents()) = ExtentForRange(3, 2);
+ *(op.add_dst_extents()) = ExtentForRange(3, 2);
+ EXPECT_TRUE(DeltaDiffGenerator::IsNoopOperation(op));
+ *(op.add_src_extents()) = ExtentForRange(7, 5);
+ *(op.add_dst_extents()) = ExtentForRange(7, 5);
+ EXPECT_TRUE(DeltaDiffGenerator::IsNoopOperation(op));
+ *(op.add_src_extents()) = ExtentForRange(20, 2);
+ *(op.add_dst_extents()) = ExtentForRange(20, 1);
+ *(op.add_dst_extents()) = ExtentForRange(21, 1);
+ EXPECT_TRUE(DeltaDiffGenerator::IsNoopOperation(op));
+ *(op.add_src_extents()) = ExtentForRange(kSparseHole, 2);
+ *(op.add_src_extents()) = ExtentForRange(kSparseHole, 1);
+ *(op.add_dst_extents()) = ExtentForRange(kSparseHole, 3);
+ EXPECT_TRUE(DeltaDiffGenerator::IsNoopOperation(op));
+ *(op.add_src_extents()) = ExtentForRange(24, 1);
+ *(op.add_dst_extents()) = ExtentForRange(25, 1);
+ EXPECT_FALSE(DeltaDiffGenerator::IsNoopOperation(op));
+}
+
} // namespace chromeos_update_engine
diff --git a/delta_performer.cc b/delta_performer.cc
index 5bb5d0a..134316e 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -119,10 +119,10 @@
if (op.src_extents_size() == 0) {
return true;
}
- // TODO(adlr): detect other types of idempotent operations. For example,
- // a MOVE may move a block onto itself.
-
- // When in doubt, it's safe to declare an op non-idempotent.
+ // When in doubt, it's safe to declare an op non-idempotent. Note that we
+ // could detect other types of idempotent operations here such as a MOVE that
+ // moves blocks onto themselves. However, we rely on the server to not send
+ // such operations at all.
ExtentRanges src_ranges;
src_ranges.AddRepeatedExtents(op.src_extents());
const uint64_t block_count = src_ranges.blocks();
diff --git a/delta_performer_unittest.cc b/delta_performer_unittest.cc
index e9d44fa..3c05b1d 100755
--- a/delta_performer_unittest.cc
+++ b/delta_performer_unittest.cc
@@ -9,6 +9,7 @@
#include <string>
#include <vector>
+#include <base/file_util.h>
#include <base/scoped_ptr.h>
#include <base/string_util.h>
#include <google/protobuf/repeated_field.h>
@@ -112,7 +113,7 @@
return true;
}
-void DoSmallImageTest(bool full_kernel) {
+void DoSmallImageTest(bool full_kernel, bool noop) {
string a_img, b_img;
EXPECT_TRUE(utils::MakeTempFile("/tmp/a_img.XXXXXX", &a_img, NULL));
ScopedPathUnlinker a_img_unlinker(a_img);
@@ -120,7 +121,6 @@
ScopedPathUnlinker b_img_unlinker(b_img);
CreateExtImageAtPath(a_img, NULL);
- CreateExtImageAtPath(b_img, NULL);
int image_size = static_cast<int>(utils::FileSize(a_img));
@@ -129,12 +129,7 @@
"dd if=/dev/zero of=%s seek=%d bs=1 count=1",
a_img.c_str(),
image_size + 1024 * 1024 - 1)));
- EXPECT_EQ(0, System(base::StringPrintf(
- "dd if=/dev/zero of=%s seek=%d bs=1 count=1",
- b_img.c_str(),
- image_size + 1024 * 1024 - 1)));
EXPECT_EQ(image_size + 1024 * 1024, utils::FileSize(a_img));
- EXPECT_EQ(image_size + 1024 * 1024, utils::FileSize(b_img));
// Make some changes to the A image.
{
@@ -154,8 +149,17 @@
ones.size()));
}
- // Make some changes to the B image.
- {
+ if (noop) {
+ EXPECT_TRUE(file_util::CopyFile(FilePath(a_img), FilePath(b_img)));
+ } else {
+ CreateExtImageAtPath(b_img, NULL);
+ EXPECT_EQ(0, System(base::StringPrintf(
+ "dd if=/dev/zero of=%s seek=%d bs=1 count=1",
+ b_img.c_str(),
+ image_size + 1024 * 1024 - 1)));
+ EXPECT_EQ(image_size + 1024 * 1024, utils::FileSize(b_img));
+
+ // Make some changes to the B image.
string b_mnt;
ScopedLoopMounter b_mounter(b_img, &b_mnt, 0);
@@ -196,6 +200,10 @@
const char* new_data_string = "This is new data.";
strcpy(&new_kernel_data[0], new_data_string);
+ if (noop) {
+ old_kernel_data = new_kernel_data;
+ }
+
// Write kernels to disk
EXPECT_TRUE(utils::WriteFile(
old_kernel.c_str(), &old_kernel_data[0], old_kernel_data.size()));
@@ -258,6 +266,11 @@
EXPECT_EQ(expected_sig_data_length, manifest.signatures_size());
EXPECT_FALSE(signature.data().empty());
+ if (noop) {
+ EXPECT_EQ(1, manifest.install_operations_size());
+ EXPECT_EQ(1, manifest.kernel_install_operations_size());
+ }
+
if (full_kernel) {
EXPECT_FALSE(manifest.has_old_kernel_info());
} else {
@@ -333,11 +346,15 @@
}
TEST(DeltaPerformerTest, RunAsRootSmallImageTest) {
- DoSmallImageTest(false);
+ DoSmallImageTest(false, false);
}
TEST(DeltaPerformerTest, RunAsRootFullKernelSmallImageTest) {
- DoSmallImageTest(true);
+ DoSmallImageTest(true, false);
+}
+
+TEST(DeltaPerformerTest, RunAsRootNoopSmallImageTest) {
+ DoSmallImageTest(false, true);
}
TEST(DeltaPerformerTest, NewFullUpdateTest) {