Remove fork-related code from Rust #7894

gshuflin · 2019-06-18T22:38:33Z

Eric-Arellano

This looks great! Holding off on approval only because I'm not enough of an authority on Rust to confidently approve.

Thanks for doing this!

src/rust/engine/src/context.rs

illicitonion

Welcome to the project! This looks great, thanks!

CI is showing that the rust code hasn't been formatted with rustfmt; can you run build-support/bin/check_rust_formatting.sh -f and commit the result?

I recommend running build-support/bin/setup.sh one time in your git checkout, which will install git hooks which will do things like check formatting before you commit :)

illicitonion · 2019-06-19T09:25:49Z

src/rust/engine/src/context.rs

-                .unwrap(),
-              remote_store_rpc_retries,
-              futures_timer_thread2.handle(),
+    let local_store_dir = local_store_dir.clone();


This clone is no longer necessary, because we're now moving the value into a FnOnce closure not an Fn closure.

illicitonion · 2019-06-19T09:29:24Z

src/rust/engine/src/lib.rs

@@ -378,13 +378,11 @@ pub extern "C" fn scheduler_metrics(
 pub extern "C" fn scheduler_fork_context(


Definitely a question for a separate PR, but I believe this is now unused, and it (and the python half which calls this) can also be deleted

Eric-Arellano · 2019-06-20T01:12:23Z

This is still failing due to error cannot allocate memory https://travis-ci.org/pantsbuild/pants/jobs/547962452#L2810.

I originally thought this was us running out of memory because of V2 using too much of it, but I don't think that's the case anymore. This failure has happened twice now for this PR, and hasn't shown up on any other builds.

@illicitonion any thoughts on what might be causing it? Greg wasn't able to reproduce locally via ./pants --no-v1 --v2 test tests/python/pants_test/engine:fs.

--

Also, has a merge conflict now.

blorente

Welcome to the project!

I don't know why the out of memory error happens, but to reproduce it locally you might want to try setting this value to something lower: https://github.com/pantsbuild/pants/blob/master/src/rust/engine/fs/src/store.rs#L24

blorente · 2019-06-20T09:11:37Z

src/rust/engine/src/context.rs

+        // Allow for some overhead for bookkeeping threads (if any).
+        process_execution_parallelism + 2,
+        store.clone(),
+        futures_timer_thread2.clone(), // TODO remove clone once the store and http are not resettables


Can we remove this clone?

illicitonion · 2019-06-20T09:48:58Z

This is still failing due to error cannot allocate memory https://travis-ci.org/pantsbuild/pants/jobs/547962452#L2810.

I originally thought this was us running out of memory because of V2 using too much of it, but I don't think that's the case anymore. This failure has happened twice now for this PR, and hasn't shown up on any other builds.

@illicitonion any thoughts on what might be causing it? Greg wasn't able to reproduce locally via ./pants --no-v1 --v2 test tests/python/pants_test/engine:fs.

I do have thoughts! So, a while ago, we made the store attempt to allocate a ridiculous amount of virtual memory. This is needed because it mmaps a potentially very large file from disk, and we want to allow the file tot be very large. We make a store per pants process, or per scheduler we create in unit tests. @stuhood ended up making us re-use schedulers across the test-cases in a test-class, because he couldn't find a convenient and reliable way to force python to gc away the schedulers (which have stores) after each test-case, and so we ended up collecting a bunch of these, and they blew up memory usage like crazy.

We currently have two separate LMDB stores, one for files (which may be large), and one for metadata about directories (which should be tiny), but we allocate the same giant amount of virtual memory to both of them. We can probably trivially drop the allocation to the one for directories by a few orders of magnitude, which I will try doing now... This is still kind of hacking around the problem, but... shrug

The reason this PR will be making us more likely to max out memory is that before Stores were only lazily initialized when they were actually needed (because that's how Resettables worked), whereas now we're eagerly initialising them every time we make a scheduler; so some schedulers we made before which didn't take a bunch of virtual memory, now will.

Also, it's not just this PR; I saw this on another of my PRs today too.

--

Also, has a merge conflict now.

I've just pushed a rebase, because that merge conflict was totally my fault, and totally foreseeable by me! Sorry for the hassle!

And all references to it

stuhood · 2019-06-21T18:29:48Z

src/rust/engine/src/context.rs

@@ -79,11 +77,10 @@ impl Core {
    let mut remote_store_servers = remote_store_servers;
    remote_store_servers.shuffle(&mut rand::thread_rng());

-    let runtime = Resettable::new(|| {
+    let runtime =
      Arc::new(RwLock::new(Runtime::new().unwrap_or_else(|e| {


Runtime is Send and Sync afaict, so the RwLock should no longer be necessary...? I wonder whether it ever was.

...oh, sorry. Except we need mutable access to it for spawn and block_on. Nevermind.

gshuflin force-pushed the remove_resettable branch from 6c688fd to a0f2e90 Compare June 18, 2019 22:39

Eric-Arellano requested review from stuhood, blorente and illicitonion June 18, 2019 22:43

Eric-Arellano reviewed Jun 18, 2019

View reviewed changes

src/rust/engine/src/context.rs Show resolved Hide resolved

illicitonion approved these changes Jun 19, 2019

View reviewed changes

gshuflin changed the title ~~Remove resettable~~ Remove fork-related code from Rust Jun 19, 2019

gshuflin force-pushed the remove_resettable branch 2 times, most recently from 57ff713 to 4e1de00 Compare June 19, 2019 23:22

blorente approved these changes Jun 20, 2019

View reviewed changes

illicitonion force-pushed the remove_resettable branch from 4e1de00 to 0f7b2aa Compare June 20, 2019 09:44

gshuflin added 3 commits June 20, 2019 14:50

Remove Resettable in context.rs

63a16c6

Remove resettable crate

4a1a252

And all references to it

Lint fix

f0abeac

illicitonion force-pushed the remove_resettable branch from 0f7b2aa to 9dd8569 Compare June 20, 2019 13:52

fmt

e2739d3

illicitonion force-pushed the remove_resettable branch from 9dd8569 to e2739d3 Compare June 20, 2019 14:52

illicitonion merged commit f7d45dc into pantsbuild:master Jun 20, 2019

gshuflin deleted the remove_resettable branch June 20, 2019 21:56

stuhood reviewed Jun 21, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove fork-related code from Rust #7894

Remove fork-related code from Rust #7894

gshuflin commented Jun 18, 2019

Eric-Arellano left a comment

illicitonion left a comment

illicitonion Jun 19, 2019

illicitonion Jun 19, 2019

Eric-Arellano commented Jun 20, 2019

blorente left a comment

blorente Jun 20, 2019

illicitonion commented Jun 20, 2019

stuhood Jun 21, 2019

stuhood Jun 21, 2019

		@@ -378,13 +378,11 @@ pub extern "C" fn scheduler_metrics(
		pub extern "C" fn scheduler_fork_context(

Remove fork-related code from Rust #7894

Remove fork-related code from Rust #7894

Conversation

gshuflin commented Jun 18, 2019

Eric-Arellano left a comment

Choose a reason for hiding this comment

illicitonion left a comment

Choose a reason for hiding this comment

illicitonion Jun 19, 2019

Choose a reason for hiding this comment

illicitonion Jun 19, 2019

Choose a reason for hiding this comment

Eric-Arellano commented Jun 20, 2019

blorente left a comment

Choose a reason for hiding this comment

blorente Jun 20, 2019

Choose a reason for hiding this comment

illicitonion commented Jun 20, 2019

stuhood Jun 21, 2019

Choose a reason for hiding this comment

stuhood Jun 21, 2019

Choose a reason for hiding this comment