Closed Bug 1543795 Opened 5 years ago Closed 5 years ago

configure LMDB to allocate less memory for ID lists

Categories

(Toolkit :: Storage, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: myk, Assigned: myk)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Bug 1538093 showed that LMDB allocates 3MB when opening an environment in read-write mode. Discussion in https://github.com/mozilla/lmdb/pull/2 suggests that we can reduce this significantly without impacting our use cases, as the default value of MDB_IDL_LOGN, which determines the sizes of the ID lists, is intended to support much larger environments (and write loads) than we expect in Firefox usage.

That pull request makes MDB_IDL_LOGN configurable in LMDB, so Firefox can specify a different value. However, in order to actually specify that value, we can't simply define it in mozilla-config.h, because rust.mk intentionally excludes that file from CFLAGS/CXXFLAGS (to avoid busting Mozilla -> Rust -> C/C++ compiles due to Mozilla-specific configuration).

We could add something like this to rust.mk, although it would be a terrible hack:

export CFLAGS_$(rust_cc_env_name) += -DMDB_IDL_LOGN=(new value)

Alternately, we could make lmdb-rkv-sys's build script define the macro if an environment variable is set:

if env::var_os("MDB_IDL_LOGN").is_some() {
    build.define("MDB_IDL_LOGN", env::var("MDB_IDL_LOGN").unwrap().as_str());
}

And then export that variable from some appropriate moz.build file.

@glandium, do you have a preferred general strategy for configuring the build of a C/C++ library that is being built by a Rust crate that is itself being built by the Mozilla build system?

Flags: needinfo?(mh+mozilla)
Blocks: 1540271

The last one is the least bad, I'd say, but I also don't know what's the most idiomatic rust way to deal with these things. It kind of sucks that rust features don't allow more than a simple toggle yes/no. I'm tempted to tell you to use rust features, one for each possible value.

Alex, what do you think?

Flags: needinfo?(mh+mozilla) → needinfo?(acrichton)
Blocks: 1515094

If different features work that's probably the best way to go, otherwise another common way we've seen is indeed the environment variable approach which is configured by the build system somewhere. Apart from those though I wouldn't have more suggestions :(

Flags: needinfo?(acrichton)
Blocks: 1544052

Configure the lmdb-rkv-sys Rust crate to use a smaller MDB_IDL_LOGN size in order to reduce allocations when opening an LMDB environment in read-write mode.

@glandium I adopted the configuration strategy you suggested of creating a "feature" for each reasonable value for the MDB_IDL_LOGN macro. Fortunately, the range of reasonable values is fairly small.

@nanj Based on your evalution in https://github.com/mozilla/lmdb/pull/2, a value of "9" for this macro should aggressively reduce the allocations while still supporting our existing use cases. But I'm open to increasing it, if you think a higher initial value would be preferable.

(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)

@nanj Based on your evalution in https://github.com/mozilla/lmdb/pull/2, a value of "9" for this macro should aggressively reduce the allocations while still supporting our existing use cases. But I'm open to increasing it, if you think a higher initial value would be preferable.

Yep, I believe that "9" is a great starting point here, which provides 4MB to track the dirty pages for every single write transaction. Even if we, though very unlikely, have big write transactions, it will merely introduce some write amplification for performing the dirty page spilling.

Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae91cf128a4c
configure lmdb-rkv-sys to use a smaller MDB_IDL_LOGN size r=nanj,glandium
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: