Closed Bug 1445451 (rkv) Opened 6 years ago Closed 6 years ago

[meta] validate, land, and use rkv

Categories

(Toolkit :: General, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: rnewman, Assigned: myk)

References

(Blocks 1 open bug)

Details

(Keywords: meta)

Attachments

(2 files, 1 obsolete file)

(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 #8958640 - Attachment description: Bug 1445451 - Vendor rkv into mozilla-central. → Bug 1445451 - Vendor rkv into mozilla-central. (WIP)
Priority: -- → P3
Assignee: bugzilla → myk
Alias: rkv
Blocks: 1460811
Attached file vendor rkv
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
Summary: [meta] rkv landing → [meta] validate, land, and use rkv
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?
Flags: needinfo?(nfroyd)
(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!
Flags: needinfo?(nfroyd)
(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
Attachment #8993453 - Flags: review+
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.
MozReview-Commit-ID: KbcADpNltYq
Comment on attachment 8998953 [details]
Bug 1445451 - vendor rkv; r=froydnj

Nathan Froyd [:froydnj] has approved the revision.
Attachment #8998953 - Flags: review+
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.
Flags: needinfo?(myk)
It sounds like an issue should be opened on that crate for its build script to do the right thing.
https://hg.mozilla.org/mozilla-central/rev/329e64cecd98
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1482810
(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.
Flags: needinfo?(mh+mozilla)
I was thinking about cargo:rerun-if-* annotations, but re-reading comment 15, that's not what's missing...
Flags: needinfo?(mh+mozilla)
Attachment #8958640 - Attachment is obsolete: true
Blocks: 1490496
Blocks: 1491169
Depends on: 1512541
Depends on: 1522609
Depends on: 1522614
Blocks: 1515094
You need to log in before you can comment on or make changes to this bug.