-
Notifications
You must be signed in to change notification settings - Fork 133
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.
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
MemoryStore now uses fixed memory pool per store #1588
base: main
Are you sure you want to change the base?
Conversation
97f0741
to
acf03e1
Compare
acf03e1
to
6e78680
Compare
6e78680
to
f70dc4e
Compare
f70dc4e
to
57983d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks fantastic, but I need to go over the new creates a bit more.
Reviewed 13 of 16 files at r1, 1 of 1 files at r2.
Reviewable status: 0 of 1 LGTMs obtained, and 14 of 17 files reviewed, and pending CI: pre-commit-checks, and 5 discussions need to be resolved
-- commits
line 7 at r1:
nit: "significantly, significantly" seems wrong.
nativelink-config/examples/basic_cas.json5
line 18 at r1 (raw file):
"memory": { "eviction_policy": { "max_bytes": 1671088640
Something seems wrong in this file. Where do these numbers come from?
Also, note that we can use human-readable values like 2gb
.
Cargo.toml
line 49 at r1 (raw file):
nativelink-metric = { path = "nativelink-metric" } nativelink-metric-collector = { path = "nativelink-metric-collector" } alloc-bytes = { path = "external-crates/alloc-bytes" }
nit: "external-crates" seems a bit odd since these crates are technically not external. What about renaming it to just crates
or third_party
or lifting these crates to the top-level directory? It seems like most projects (like tokio and crossbeam) just keep all crates at the root and I'd lean towards this as well to avoid unnecessary directory nesting. This also allows convenient commands like bazel test alloc-bytes
where alloc-bytes
is tab-completed from the directory name and automatically expands to bazel test //alloc-bytes:alloc-bytes
.
external-crates/alloc-bytes/Cargo.toml
line 13 at r1 (raw file):
description = "A lightweight crate offering custom-allocated byte buffers with user-specified allocators for efficient, fine-tuned memory management." keywords = ["bytes", "allocator", "buffers"] categories = ["network-programming", "data-structures"]
nit: Remember to double-check that we're using all relevant keywords and categories here.
external-crates/heap-allocator/Cargo.toml
line 7 at r1 (raw file):
authors = [ "Nativelink Authors", "Nathan (Blaise) Bruer <[email protected]>"
nit: I'm noticing custom email addresses. Is this intentional?
57983d0
to
62a6683
Compare
62a6683
to
7cf70c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 LGTMs obtained, and 10 of 20 files reviewed, and pending CI: pre-commit-checks, and 4 discussions need to be resolved
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: "significantly, significantly" seems wrong.
Done.
Cargo.toml
line 49 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: "external-crates" seems a bit odd since these crates are technically not external. What about renaming it to just
crates
orthird_party
or lifting these crates to the top-level directory? It seems like most projects (like tokio and crossbeam) just keep all crates at the root and I'd lean towards this as well to avoid unnecessary directory nesting. This also allows convenient commands likebazel test alloc-bytes
wherealloc-bytes
is tab-completed from the directory name and automatically expands tobazel test //alloc-bytes:alloc-bytes
.
Lets discuss this in person. The difference between tokio
and crossbeam
is that their crates are all part of the root project... like their root project is a library, so their top-level stuff are also libraries... but for us, this is just a library of the application.
external-crates/alloc-bytes/Cargo.toml
line 13 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: Remember to double-check that we're using all relevant keywords and categories here.
Yeah, this is the same categories bytes
uses.
external-crates/heap-allocator/Cargo.toml
line 7 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: I'm noticing custom email addresses. Is this intentional?
Yeah, I capture everything before blaise
in my personal email.
nativelink-config/examples/basic_cas.json5
line 18 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
Something seems wrong in this file. Where do these numbers come from?
Also, note that we can use human-readable values like
2gb
.
Ooops, this should not be part of the commit.
7cf70c2
to
8607816
Compare
MemoryStore now pre-allocates max_bytes from the eviction policy, then installs a custom allocator in that pool that the store will use. In testing, memory fragmentation appears to have dropped significantly decreasing the number of sys-calls managing memory and slightly reduced user time. In addition, we remove two full copies of our data when we store it in the memory store. This PR also adds two new crates (likely to be published). "Heap Allocator" Fixed heap allocator based on the TLSF (Two-Level Segregated Fit) algorithm. TLSF is a fast, constant-time memory allocation algorithm designed for real-time applications. It organizes free memory into segregated lists based on block sizes, allowing for quick allocation and deallocation with minimal fragmentation. "Alloc Bytes" Provides custom byte buffers allocated using a user-provided allocator. It offers both mutable and immutable byte buffers, enabling efficient memory allocation strategies in systems with custom memory requirements. closes: TraceMachina#289
8607816
to
f9781e8
Compare
MemoryStore now pre-allocates max_bytes from the eviction policy,
then installs a custom allocator in that pool that the store will
use. In testing, memory fragmentation appears to have dropped
significantly, significantly decreased the number of sys-calls
managing memory and slightly reduced user time.
In addition, we remove two full copies of our data when we store
it in the memory store.
This PR also adds two new crates (likely to be published).
"Heap Allocator"
Fixed heap allocator based on the TLSF (Two-Level Segregated
Fit) algorithm. TLSF is a fast, constant-time memory allocation
algorithm designed for real-time applications. It organizes free
memory into segregated lists based on block sizes, allowing for
quick allocation and deallocation with minimal fragmentation.
"Alloc Bytes"
Provides custom byte buffers allocated using a user-provided
allocator. It offers both mutable and immutable byte buffers,
enabling efficient memory allocation strategies in systems with
custom memory requirements.
closes: #289
This change is