From eaf5c49dc43dd37d36cdc1bb6e4b6d0142948f29 Mon Sep 17 00:00:00 2001 From: Thomas Orozco Date: Wed, 17 Jul 2019 08:09:59 -0700 Subject: [PATCH] mononoke: repo_commit: deduplicate envelope fetch Summary: We are fetching envelopes twice in repo_commit, this updates the code to provide a shared abstraction over file and manifest envelopes in order to avoid this duplicate fetch. Reviewed By: farnz Differential Revision: D16282794 fbshipit-source-id: 53d53f28f051cbacbc1c931b2dbb98ac74a19c15 --- blobrepo/src/envelope.rs | 48 +++++++++++++++++++++++++++++++++++ blobrepo/src/file.rs | 48 ++++++++++------------------------- blobrepo/src/lib.rs | 1 + blobrepo/src/repo_commit.rs | 50 ++++++++++++++++++------------------- 4 files changed, 87 insertions(+), 60 deletions(-) create mode 100644 blobrepo/src/envelope.rs diff --git a/blobrepo/src/envelope.rs b/blobrepo/src/envelope.rs new file mode 100644 index 0000000000..46d61c5690 --- /dev/null +++ b/blobrepo/src/envelope.rs @@ -0,0 +1,48 @@ +// Copyright (c) 2004-present, Facebook, Inc. +// All Rights Reserved. +// +// This software may be used and distributed according to the terms of the +// GNU General Public License version 2 or any later version. + +use crate::file::get_rename_from_envelope; +use failure_ext::Result; +use mercurial_types::{HgFileEnvelope, HgFileNodeId, HgManifestEnvelope, HgParents, MPath}; + +pub trait HgBlobEnvelope { + fn get_parents(&self) -> HgParents; + fn get_copy_info(&self) -> Result>; + fn get_size(&self) -> Option; +} + +impl HgBlobEnvelope for HgFileEnvelope { + fn get_parents(&self) -> HgParents { + let (p1, p2) = self.parents(); + HgParents::new( + p1.map(HgFileNodeId::into_nodehash), + p2.map(HgFileNodeId::into_nodehash), + ) + } + + fn get_copy_info(&self) -> Result> { + get_rename_from_envelope(self.clone()) + } + + fn get_size(&self) -> Option { + Some(self.content_size()) + } +} + +impl HgBlobEnvelope for HgManifestEnvelope { + fn get_parents(&self) -> HgParents { + let (p1, p2) = self.parents(); + HgParents::new(p1, p2) + } + + fn get_copy_info(&self) -> Result> { + Ok(None) + } + + fn get_size(&self) -> Option { + None + } +} diff --git a/blobrepo/src/file.rs b/blobrepo/src/file.rs index 1ca5bfd13f..9ac45664b0 100644 --- a/blobrepo/src/file.rs +++ b/blobrepo/src/file.rs @@ -7,12 +7,13 @@ //! Plain files, symlinks use super::alias::get_sha256; +use crate::envelope::HgBlobEnvelope; use crate::errors::*; use crate::manifest::{fetch_manifest_envelope, fetch_raw_manifest_bytes, BlobManifest}; use blobstore::Blobstore; use context::CoreContext; use failure_ext::{bail_err, bail_msg, Error, FutureFailureErrorExt}; -use futures::future::{self, lazy, Future}; +use futures::future::{lazy, Future}; use futures_ext::{BoxFuture, FutureExt}; use mercurial::file; use mercurial_types::manifest::{Content, Entry, Manifest, Type}; @@ -255,16 +256,16 @@ impl HgBlobEntry { } } - pub fn get_copy_info( - &self, - ctx: CoreContext, - ) -> impl Future, Error = Error> { + pub fn get_envelope(&self, ctx: CoreContext) -> BoxFuture, Error> { match self.id { - HgEntryId::Manifest(_) => future::ok(None).left_future(), - HgEntryId::File(_, hash) => fetch_file_envelope(ctx.clone(), &self.blobstore, hash) - .and_then(get_rename_from_envelope) + HgEntryId::Manifest(hash) => fetch_manifest_envelope(ctx, &self.blobstore, hash) + .map(|e| Box::new(e) as Box) + .left_future(), + HgEntryId::File(_, hash) => fetch_file_envelope(ctx, &self.blobstore, hash) + .map(|e| Box::new(e) as Box) .right_future(), } + .boxify() } } @@ -274,25 +275,7 @@ impl Entry for HgBlobEntry { } fn get_parents(&self, ctx: CoreContext) -> BoxFuture { - match self.id { - HgEntryId::Manifest(hash) => { - fetch_manifest_envelope(ctx.clone(), &self.blobstore, hash) - .map(move |envelope| { - let (p1, p2) = envelope.parents(); - HgParents::new(p1, p2) - }) - .boxify() - } - HgEntryId::File(_, hash) => fetch_file_envelope(ctx.clone(), &self.blobstore, hash) - .map(move |envelope| { - let (p1, p2) = envelope.parents(); - HgParents::new( - p1.map(HgFileNodeId::into_nodehash), - p2.map(HgFileNodeId::into_nodehash), - ) - }) - .boxify(), - } + self.get_envelope(ctx).map(|e| e.get_parents()).boxify() } fn get_raw_content(&self, ctx: CoreContext) -> BoxFuture { @@ -349,14 +332,9 @@ impl Entry for HgBlobEntry { // XXX get_size should probably return a u64, not a usize fn get_size(&self, ctx: CoreContext) -> BoxFuture, Error> { - match self.id { - HgEntryId::Manifest(_) => future::ok(None).boxify(), - HgEntryId::File(_, filenode_id) => { - fetch_file_envelope(ctx.clone(), &self.blobstore, filenode_id) - .map(|envelope| Some(envelope.content_size() as usize)) - .boxify() - } - } + self.get_envelope(ctx) + .map(|e| e.get_size().map(|s| s as usize)) + .boxify() } fn get_hash(&self) -> HgEntryId { diff --git a/blobrepo/src/lib.rs b/blobrepo/src/lib.rs index 5c7579cdf5..9879d26740 100644 --- a/blobrepo/src/lib.rs +++ b/blobrepo/src/lib.rs @@ -15,6 +15,7 @@ pub mod alias; mod bonsai_generation; pub mod derive_hg_manifest; +mod envelope; pub mod file; pub mod file_history; mod manifest; diff --git a/blobrepo/src/repo_commit.rs b/blobrepo/src/repo_commit.rs index c0af8c75ea..08ee07f065 100644 --- a/blobrepo/src/repo_commit.rs +++ b/blobrepo/src/repo_commit.rs @@ -35,6 +35,7 @@ use mercurial_types::{ use mononoke_types::{self, BonsaiChangeset, ChangesetId, RepositoryId}; use stats::define_stats; +use crate::envelope::HgBlobEnvelope; use crate::errors::*; use crate::file::HgBlobEntry; use crate::BlobRepo; @@ -425,25 +426,26 @@ impl UploadEntries { future::ok(()).left_future() } else { let filenodeinfos = stream::futures_unordered(uploaded_entries.into_iter().map( - // TODO: We are duplicating the envelope fetch here. |(path, blobentry)| { - ( - blobentry.get_parents(ctx.clone()), - compute_copy_from_info(ctx.clone(), &path, &blobentry), - ) - .into_future() - .map(move |(parents, copyfrom)| { - let (p1, p2) = parents.get_nodes(); - FilenodeInfo { - path, - filenode: HgFileNodeId::new( - blobentry.get_hash().into_nodehash(), - ), - p1: p1.map(HgFileNodeId::new), - p2: p2.map(HgFileNodeId::new), - copyfrom, - linknode: HgChangesetId::new(cs_id), - } + blobentry + .get_envelope(ctx.clone()) + .and_then(move |envelope| { + let parents = envelope.get_parents(); + let copy_from = compute_copy_from_info(&path, &envelope); + + copy_from.map(move |copy_from| { + let (p1, p2) = parents.get_nodes(); + FilenodeInfo { + path, + filenode: HgFileNodeId::new( + blobentry.get_hash().into_nodehash(), + ), + p1: p1.map(HgFileNodeId::new), + p2: p2.map(HgFileNodeId::new), + copyfrom: copy_from, + linknode: HgChangesetId::new(cs_id), + } + }) }) }, )) @@ -474,21 +476,19 @@ impl UploadEntries { } fn compute_copy_from_info( - ctx: CoreContext, path: &RepoPath, - blobentry: &HgBlobEntry, -) -> BoxFuture, Error> { + envelope: &Box, +) -> Result> { match path { &RepoPath::FilePath(_) => { STATS::finalize_compute_copy_from_info.add_value(1); - blobentry - .get_copy_info(ctx) + envelope + .get_copy_info() .map(|copiedfrom| copiedfrom.map(|(path, node)| (RepoPath::FilePath(path), node))) - .boxify() } &RepoPath::RootPath | &RepoPath::DirectoryPath(_) => { // No copy information for directories/repo roots - Ok(None).into_future().boxify() + Ok(None) } } }