Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch to Thrift in SimpleOpStore #700

Merged
merged 2 commits into from Nov 13, 2022
Merged

Switch to Thrift in SimpleOpStore #700

merged 2 commits into from Nov 13, 2022

Conversation

martinvonz
Copy link
Owner

@martinvonz martinvonz commented Nov 3, 2022 •
edited

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)

@martinvonz martinvonz force-pushed the push-4bddffe180d445be9d82618a5bc7e91c branch 3 times, most recently from 9b7b089 to 3882777 Compare November 3, 2022 17:55
chooglen
chooglen reviewed Nov 3, 2022
View reviewed changes
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
@martinvonz martinvonz force-pushed the push-4bddffe180d445be9d82618a5bc7e91c branch 3 times, most recently from 27ba2e8 to 76e2643 Compare November 5, 2022 13:41
Ralith
Ralith reviewed Nov 7, 2022
View reviewed changes
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
lib/src/simple_op_store.rs Show resolved Hide resolved
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
lib/src/simple_op_store_model.thrift Show resolved Hide resolved
lib/src/simple_op_store_model.thrift Outdated Show resolved Hide resolved
Ralith
Ralith reviewed Nov 7, 2022
View reviewed changes
lib/Cargo.toml Outdated Show resolved Hide resolved
@martinvonz martinvonz force-pushed the push-4bddffe180d445be9d82618a5bc7e91c branch 3 times, most recently from ea3a114 to 34b21a6 Compare November 9, 2022 00:37
@martinvonz martinvonz mentioned this pull request Nov 9, 2022
Closed
2 tasks
Ralith
Ralith reviewed Nov 9, 2022
View reviewed changes
Copy link
Collaborator

@Ralith Ralith left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hand-written hash impls seem verbose, error-prone, and high risk. Should we pursue automating them with a derive macro in immediate future work? I'd be happy to take that on, but it would likely result in different hashes. Prior ecosystem discussion in RustCrypto/utils#2.

lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
@martinvonz martinvonz force-pushed the push-4bddffe180d445be9d82618a5bc7e91c branch 3 times, most recently from f254556 to da38602 Compare November 9, 2022 17:30
@martinvonz
Copy link
Owner Author

The hand-written hash impls seem verbose, error-prone, and high risk.

Yeah, I agree. I feel much better about them with the change to include the length as prefix, though. Now the pattern is clearer.

Should we pursue automating them with a derive macro in immediate future work? I'd be happy to take that on, but it would likely result in different hashes. Prior ecosystem discussion in RustCrypto/utils#2.

Changing the hashes again would be unfortunate, so if we're going to write macros for generating hashes, I think we should let this PR wait. We're going to have to do the same migration for the native commit backend. If you wrote macros for generating content hashes, that could be used there too.

Ralith
Ralith approved these changes Nov 9, 2022
View reviewed changes
Copy link
Collaborator

@Ralith Ralith left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'll try to bang out a proc macro for the hashing within the next few days (unless it's not worth the delay).

@martinvonz
Copy link
Owner Author

LGTM, but I'll try to bang out a proc macro for the hashing within the next few days (unless it's not worth the delay).

Thanks! I think it's fine to wait.

@martinvonz martinvonz force-pushed the push-4bddffe180d445be9d82618a5bc7e91c branch from da38602 to 9c59640 Compare November 10, 2022 06:52
@Ralith Ralith mentioned this pull request Nov 12, 2022
Merged
@martinvonz
op_store: extract current Protobuf-based implementation to separate file
d490e2d 
In order to allow building jj inside of Google, our Protobuf team
doesn't want to us to use a Google-unsupported implementation. Since
there is no supported implementation in Rust, we have to migrate off
of Protobufs. I'm starting with the operation store. This commit moves
the current implementation to a separate file so it can easily be
disabled by a Caargo feature.
@martinvonz martinvonz force-pushed the push-4bddffe180d445be9d82618a5bc7e91c branch from 9c59640 to 444dca6 Compare November 13, 2022 06:48
@martinvonz
Copy link
Owner Author

LGTM, but I'll try to bang out a proc macro for the hashing within the next few days (unless it's not worth the delay).

Thanks! I think it's fine to wait.

This is now ready for review again!

@martinvonz
Copy link
Owner Author

LGTM, but I'll try to bang out a proc macro for the hashing within the next few days (unless it's not worth the delay).

Thanks! I think it's fine to wait.

This is now ready for review again!

Eh, I forgot to actually use the new content hashing in the new ThriftOpStore. Just a minute...

@martinvonz martinvonz force-pushed the push-4bddffe180d445be9d82618a5bc7e91c branch from 444dca6 to 25a79ea Compare November 13, 2022 07:09
@martinvonz
Copy link
Owner Author

This is now ready for review again!

Eh, I forgot to actually use the new content hashing in the new ThriftOpStore. Just a minute...

Alright, hopefully it looks better now.

Ralith
Ralith approved these changes Nov 13, 2022
View reviewed changes
lib/src/proto_op_store.rs Outdated Show resolved Hide resolved
lib/src/lib.rs Outdated Show resolved Hide resolved
@martinvonz martinvonz force-pushed the push-4bddffe180d445be9d82618a5bc7e91c branch from 25a79ea to 6b90c00 Compare November 13, 2022 19:09
@martinvonz martinvonz enabled auto-merge (rebase) November 13, 2022 19:11
@martinvonz
simple_op_store: replace Protobuf by Thrift
f74387c 
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 martinvonz force-pushed the push-4bddffe180d445be9d82618a5bc7e91c branch from 6b90c00 to f74387c Compare November 13, 2022 19:25
@martinvonz martinvonz merged commit 4ee2617 into main Nov 13, 2022
@martinvonz martinvonz deleted the push-4bddffe180d445be9d82618a5bc7e91c branch November 13, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chooglen chooglen chooglen left review comments

@Ralith Ralith Ralith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@martinvonz @Ralith @chooglen