(Not filing this under "Storage", 'cos that doesn't mean what you might think it means.) We're in the process of considering landing a generic key-value store to consolidate a number of existing special-purpose storage layers, and to provide new capabilities for future features. This bug isn't to discuss any of that; the review process happens outside Bugzilla. This bug is to track machinery for landing and preparing the library for use, should we decide to proceed. That will involve: - Whitelisting the licenses for any dependencies (Bug 1445341) - Vendoring some Rust code: - The library itself - Its small set of dependencies (which might require build peer approval, rather than --build-peers-said-large-imports-were-ok) - Implementing a FFI (probably in the upstream repo) for use from C++. Initial estimates after building my stack of patches show almost zero impact on installer size (likely because of LTO) and about 1% increase in libgkrust.a, some of which is from libraries that we already plan to use elsewhere (e.g., uuid, ordered-float, failure). LMDB itself is about 32KB of object code, and the Rust wrapper code itself doesn't add too much.

Attachment 8993453 [details] rebases rnewman's patch against current mozilla-central, removes usage of rkv from it (so that it only vendors rkv), and aligns the Rust dependencies between rkv and toolkit/, making this change to toolkit/: * upgrade uuid from 0.1.18 to 0.5 And these changes to rkv (currently pending in https://github.com/mozilla/rkv/pull/57): * upgrade bincode from 0.9 to 1.0 * switch from ordered-float to new-ordered-float * upgrade lmdb from 0.7 to 0.8 (to align its bitflags sub-dependency with Gecko) On my Mac, building today's central branch results in libxul and libgkrust.a binaries with these sizes: 453088004 obj-x86_64-apple-darwin17.7.0/toolkit/library/x86_64-apple-darwin/release/libgkrust.a 226692808 obj-x86_64-apple-darwin17.7.0/toolkit/library/XUL With this patch, both libraries actually shrink, presumably because of the uuid upgrade: 452049580 obj-x86_64-apple-darwin17.7.0/toolkit/library/x86_64-apple-darwin/release/libgkrust.a 226201936 obj-x86_64-apple-darwin17.7.0/toolkit/library/XUL libgkrust.a: vendor-rkv - central = 452049580 - 453088004 = -1,038,424 XUL: vendor-rkv - central = 226201936 - 226692808 = -490,872 If I add back usage, libgkrust.a increases in size relative to central, but libxul is still smaller: 454194772 obj-x86_64-apple-darwin17.7.0/toolkit/library/x86_64-apple-darwin/release/libgkrust.a 226454948 obj-x86_64-apple-darwin17.7.0/toolkit/library/XUL libgkrust.a: vendor-rkv - central = 454194772 - 453088004 = 1,106,768 XUL: vendor-rkv - central = 226454948 - 226692808 = -237,860

Including the dependency alignment described in comment #3, Here's the full set of mozilla-central Rust crate dependency changes that would result from vendoring rkv: New crates: * arrayref * failure * failure_derive * lmdb * lmdb-sys * rkv itself, of course * synom New versions of existing crates: * syn 0.11.11 (0.13.1, 0.14.2 are in tree) * synstructure 0.6.1 (0.8.1 is in tree) * unicode-xid 0.0.4 (0.1.0 is in tree) Updated version of existing crate: * uuid updated from 0.1.18 to 0.5.1 Note that https://github.com/rust-lang-nursery/failure/issues/209 describes an imminent update from 0.1.1 to 0.1.2 for both failure and failure_derive that will bump the latter's syn and synstructure dependencies to 0.14 and 0.9, respectively. That would align the syn and unicode-xid dependencies with an existing version in tree, and it would remove the synom dependency. As for synstructure, the only code that depends on version 0.8 in tree is Servo, so we might be able to align it by upgrading Servo to synstructure 0.9. Nathan, in <https://groups.google.com/d/msg/mozilla.dev.platform/oyRvAcynt2M/BUZ56s7WBwAJ>, regarding Rust crate review, you wrote, "I think you should request review from the people who would normally review your code." And my plan to date has been to land rkv in conjunction with one or more consumers of it. But in <https://groups.google.com/d/msg/mozilla.dev.platform/oyRvAcynt2M/NJlYTlZ2CAAJ>, Ted proposes we "make sure that someone has reviewed each new vendored crate in a bug separate from the one with the patch that adds code using it." And there'd be some value to having rkv in-tree, as it'd enable the various teams that have expressed an interest in using rkv to develop consumers on branches and behind pref flags. If we were to pursue that approach (landing rkv here and usage in other bugs), then these changes would be confined to Cargo.toml, Cargo.lock, third_party/rust/, and a few scattered changes around the tree (due to dependency alignment). Are you amenable to that approach, and would you and/or Ehsan be the appropriate reviewers for it?

(In reply to Myk Melez [:myk] [@mykmelez] from comment #4) > But in > <https://groups.google.com/d/msg/mozilla.dev.platform/oyRvAcynt2M/ > NJlYTlZ2CAAJ>, Ted proposes we "make sure that someone has reviewed each new > vendored crate in a bug separate from the one with the patch that adds code > using it." > > And there'd be some value to having rkv in-tree, as it'd enable the various > teams that have expressed an interest in using rkv to develop consumers on > branches and behind pref flags. > > If we were to pursue that approach (landing rkv here and usage in other > bugs), then these changes would be confined to Cargo.toml, Cargo.lock, > third_party/rust/, and a few scattered changes around the tree (due to > dependency alignment). Are you amenable to that approach, and would you > and/or Ehsan be the appropriate reviewers for it? FTR, I think either way works, but inspecting dependent crates is probably easier with the separate bugs approach. Let's do the separate bugs approach, since landing rkv itself unblocks other work as well. I think Ehsan or I would be fine choices for reviewers or we can find other people if we feel out of our depth. Thanks!

(In reply to Myk Melez [:myk] [@mykmelez] from comment #4) > Note that https://github.com/rust-lang-nursery/failure/issues/209 describes > an imminent update from 0.1.1 to 0.1.2 for both failure and failure_derive > that will bump the latter's syn and synstructure dependencies to 0.14 and > 0.9, respectively. > > That would align the syn and unicode-xid dependencies with an existing > version in tree, and it would remove the synom dependency. As for > synstructure, the only code that depends on version 0.8 in tree is Servo, so > we might be able to align it by upgrading Servo to synstructure 0.9. failure and failure_derive were updated today, and I updated rkv to use the new versions, which aligned their dependencies with existing versions of syn and unicode-xid in mozilla-central. The new set of mozilla-central Rust crate dependency changes that would result from vendoring rkv are: New crates: * arrayref * failure * failure_derive * lmdb * lmdb-sys * rkv itself, of course New versions of existing crates: * synstructure 0.9.0 (0.8.1 is in tree) Updated versions of existing crates: * proc-macro2 0.4 updated from 0.4.6 to 0.4.9 * syn 0.14 updated from 0.14.2 to 0.14.6 * uuid updated from 0.1.18 to 0.5.1 The only additional dependency alignment we could do is to update the synstructure dependency from 0.8 to 0.9 in servo/components/malloc_size_of_derive and servo/components/style_derive. That'd require us to also update their syn dependency from 0.13 to 0.14, and it's unclear to me how large a change that would be, so I haven't tried to do it in this patch. I also updated to the latest versions of rkv and lmdb, and I've specified rkv and lmdb dependencies as a Git repo/revision combo rather than a crates.io version number, because we've forked the lmdb crate to take some fixes for rkv, and we're still debating the advantages and drawbacks of publishing that fork to crates.io. Nathan, you provided feedback on the approach earlier, so perhaps you're also the right person to review this patch (or delegate review as appropriate). I'll request that in Phabricator.

Comment on attachment 8993453 [details] vendor rkv Nathan Froyd [:froydnj] has approved the revision. https://phabricator.services.mozilla.com/D2246

Pushed by myk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/08fa47a24e89 vendor rkv r=froydnj

Backout by dvarga@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6d19dd7ed3e5 Backed out changeset 08fa47a24e89 for failing Btup

The change was backed out for failing Btup, which is categorized as tier-2 and presumably testing the experimental tup build backend. I'm looking into the build failure as well as the (mis?) categorization of Btup.

Backed out for perma-failing Btup builds Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=08fa47a24e89697e4e43177860b55cc28298bbff Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=192868123&repo=autoland&lineNumber=2153 Backout: https://hg.mozilla.org/integration/autoland/rev/6d19dd7ed3e564b88ab5b962e90ebf73c21fc257

MozReview-Commit-ID: KbcADpNltYq

Comment on attachment 8998953 [details] Bug 1445451 - vendor rkv; r=froydnj Nathan Froyd [:froydnj] has approved the revision.

Pushed by myk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/329e64cecd98 vendor rkv; r=froydnj

The Btup jobs failed because liblmdb.a was "written to, but is not in .tup/db. You probably should specify it as an output." So too for midl.o and mdb.o. The fix, per mshal (cc:ed) on IRC, is to specify the build outputs of lmdb-sys's custom build script (third_party/rust/lmdb-sys/build.rs) in python/mozbuild/mozbuild/backend/cargo_build_defs.py. I did so, confirmed it fixed the tup build backend locally (and the Btup job on tryserver), and pushed a new Phabricator revision (as it doesn't seem possible to re-open the original one). froydnj then accepted the revision, and I lando-ed it.

It sounds like an issue should be opened on that crate for its build script to do the right thing.

(In reply to Mike Hommey [:glandium] from comment #16) > It sounds like an issue should be opened on that crate for its build script > to do the right thing. Mike, can you elaborate? I'm not very familiar with build scripts for Rust crates, and it isn't clear to me what this one is doing wrong.

I was thinking about cargo:rerun-if-* annotations, but re-reading comment 15, that's not what's missing...