simple_op_store: replace Protobuf by Thrift

As mentioned in the previous commit, we need to remove the Protobuf dependency in order to be allowed to import jj into Google's repo. This commit makes `SimpleOpStore` store its data using Thrift instead of Protobufs. It also adds automatic upgrade of existing repos. The upgrade process took 18 s in my repo, which has 22k operations. The upgraded storage uses practically the same amount of space. `jj op log` (the full outut) in my repo slowed down from 1.2 s to 3.4 s. Luckily that's an uncommon operation. I couldn't measure any difference in `jj status` (loading a single operation).
martinvonz · Nov 13, 2022 · 4ee2617 · 4ee2617
1 parent 8acb54b
commit 4ee2617
Show file tree

Hide file tree

Showing 9 changed files with 1,716 additions and 118 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -22,6 +22,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   description, even if there already was a description set. It now also only
   works on the working-copy commit (there's no `-r` argument).
 
+* The storage format for the operation log has changed. It will be
+  automatically upgraded the first time you run a command in an existing repo.
+  The operation IDs will change in that process.
+
 ### New features
 
 * The new `jj git remote rename` command allows git remotes to be renamed

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/flake.nix b/flake.nix
@@ -43,6 +43,9 @@
 
               cargoLock = {
                 lockFile = "${self}/Cargo.lock";
+                outputHashes = {
+                  "thrift-0.17.0" = "sha256-Zczwq6zRKPXXj7JR0X/0Osl1Lafo5r+2wK5tuWJbvI8=";
+                };
               };
               nativeBuildInputs = [
                 pkg-config gzip makeWrapper

diff --git a/lib/Cargo.toml b/lib/Cargo.toml
@@ -38,6 +38,8 @@ regex = "1.7.0"
 serde_json = "1.0.87"
 tempfile = "3.3.0"
 thiserror = "1.0.37"
+# thrift v0.17.0 (specified by hash for security reasons)
+thrift = { git = "https://github.com/apache/thrift", rev = "4d493e867b349f3475203ef9848353b315203c51",  default-features = false }
 uuid = { version = "1.2.1", features = ["v4"] }
 whoami = "1.2.3"
 zstd = "0.11.2"
@@ -50,4 +52,8 @@ test-case = "2.2.2"
 testutils = { path = "testutils" }
 
 [features]
+default = ["legacy_protobuf"]
 vendored-openssl = ["git2/vendored-openssl"]
+# Enable upgrade of repositories created with storage backends based on
+# Protobuf format (from before we switched to Thrift)
+legacy_protobuf = []
diff --git a/lib/src/lib.rs b/lib/src/lib.rs
@@ -37,6 +37,7 @@ pub mod nightly_shims;
 pub mod op_heads_store;
 pub mod op_store;
 pub mod operation;
+#[cfg(feature = "legacy_protobuf")]
 mod proto_op_store;
 pub mod protos;
 pub mod refs;
@@ -47,6 +48,7 @@ pub mod revset_graph_iterator;
 pub mod rewrite;
 pub mod settings;
 pub mod simple_op_store;
+mod simple_op_store_model;
 pub mod stacked_table;
 pub mod store;
 pub mod transaction;

diff --git a/lib/src/proto_op_store.rs b/lib/src/proto_op_store.rs
@@ -14,15 +14,14 @@
 
 use std::collections::BTreeMap;
 use std::fmt::Debug;
-use std::fs;
 use std::fs::File;
 use std::io::ErrorKind;
 use std::path::PathBuf;
 
 use blake2::Blake2b512;
 use itertools::Itertools;
 use protobuf::{Message, MessageField};
-use tempfile::{NamedTempFile, PersistError};
+use tempfile::NamedTempFile;
 
 use crate::backend::{CommitId, MillisSinceEpoch, Timestamp};
 use crate::content_hash::ContentHash;
@@ -32,18 +31,6 @@ use crate::op_store::{
     RefTarget, View, ViewId, WorkspaceId,
 };
 
-impl From<std::io::Error> for OpStoreError {
-    fn from(err: std::io::Error) -> Self {
-        OpStoreError::Other(err.to_string())
-    }
-}
-
-impl From<PersistError> for OpStoreError {
-    fn from(err: PersistError) -> Self {
-        OpStoreError::Other(err.to_string())
-    }
-}
-
 impl From<protobuf::Error> for OpStoreError {
     fn from(err: protobuf::Error) -> Self {
         OpStoreError::Other(err.to_string())
@@ -56,12 +43,6 @@ pub struct ProtoOpStore {
 }
 
 impl ProtoOpStore {
-    pub fn init(store_path: PathBuf) -> Self {
-        fs::create_dir(store_path.join("views")).unwrap();
-        fs::create_dir(store_path.join("operations")).unwrap();
-        Self::load(store_path)
-    }
-
     pub fn load(store_path: PathBuf) -> Self {
         ProtoOpStore { path: store_path }
     }
@@ -364,97 +345,3 @@ fn hash(x: &impl ContentHash) -> digest::Output<Blake2b512> {
     x.hash(&mut hasher);
     hasher.finalize()
 }
-
-#[cfg(test)]
-mod tests {
-    use maplit::{btreemap, hashmap, hashset};
-
-    use super::*;
-
-    #[test]
-    fn test_read_write_view() {
-        let temp_dir = testutils::new_temp_dir();
-        let store = ProtoOpStore::init(temp_dir.path().to_owned());
-        let head_id1 = CommitId::from_hex("aaa111");
-        let head_id2 = CommitId::from_hex("aaa222");
-        let public_head_id1 = CommitId::from_hex("bbb444");
-        let public_head_id2 = CommitId::from_hex("bbb555");
-        let branch_main_local_target = RefTarget::Normal(CommitId::from_hex("ccc111"));
-        let branch_main_origin_target = RefTarget::Normal(CommitId::from_hex("ccc222"));
-        let branch_deleted_origin_target = RefTarget::Normal(CommitId::from_hex("ccc333"));
-        let tag_v1_target = RefTarget::Normal(CommitId::from_hex("ddd111"));
-        let git_refs_main_target = RefTarget::Normal(CommitId::from_hex("fff111"));
-        let git_refs_feature_target = RefTarget::Conflict {
-            removes: vec![CommitId::from_hex("fff111")],
-            adds: vec![CommitId::from_hex("fff222"), CommitId::from_hex("fff333")],
-        };
-        let default_wc_commit_id = CommitId::from_hex("abc111");
-        let test_wc_commit_id = CommitId::from_hex("abc222");
-        let view = View {
-            head_ids: hashset! {head_id1, head_id2},
-            public_head_ids: hashset! {public_head_id1, public_head_id2},
-            branches: btreemap! {
-                "main".to_string() => BranchTarget {
-                    local_target: Some(branch_main_local_target),
-                    remote_targets: btreemap! {
-                        "origin".to_string() => branch_main_origin_target,
-                    }
-                },
-                "deleted".to_string() => BranchTarget {
-                    local_target: None,
-                    remote_targets: btreemap! {
-                        "origin".to_string() => branch_deleted_origin_target,
-                    }
-                },
-            },
-            tags: btreemap! {
-                "v1.0".to_string() => tag_v1_target,
-            },
-            git_refs: btreemap! {
-                "refs/heads/main".to_string() => git_refs_main_target,
-                "refs/heads/feature".to_string() => git_refs_feature_target
-            },
-            git_head: Some(CommitId::from_hex("fff111")),
-            wc_commit_ids: hashmap! {
-                WorkspaceId::default() => default_wc_commit_id,
-                WorkspaceId::new("test".to_string()) => test_wc_commit_id,
-            },
-        };
-        let view_id = store.write_view(&view).unwrap();
-        let read_view = store.read_view(&view_id).unwrap();
-        assert_eq!(read_view, view);
-    }
-
-    #[test]
-    fn test_read_write_operation() {
-        let temp_dir = testutils::new_temp_dir();
-        let store = ProtoOpStore::init(temp_dir.path().to_owned());
-        let operation = Operation {
-            view_id: ViewId::from_hex("aaa111"),
-            parents: vec![
-                OperationId::from_hex("bbb111"),
-                OperationId::from_hex("bbb222"),
-            ],
-            metadata: OperationMetadata {
-                start_time: Timestamp {
-                    timestamp: MillisSinceEpoch(123456789),
-                    tz_offset: 3600,
-                },
-                end_time: Timestamp {
-                    timestamp: MillisSinceEpoch(123456800),
-                    tz_offset: 3600,
-                },
-                description: "check out foo".to_string(),
-                hostname: "some.host.example.com".to_string(),
-                username: "someone".to_string(),
-                tags: hashmap! {
-                    "key1".to_string() => "value1".to_string(),
-                    "key2".to_string() => "value2".to_string(),
-                },
-            },
-        };
-        let op_id = store.write_operation(&operation).unwrap();
-        let read_operation = store.read_operation(&op_id).unwrap();
-        assert_eq!(read_operation, operation);
-    }
-}