885523 - Perform tree clobbers in the background on OS X

Status: RESOLVED WONTFIX

(Firefox Build System :: Mach Core, enhancement)

Description

[Adam Roach [:abr]]

Currently, tree clobbers on OS X can take non-trivial time. Rather than
holding up the build waiting for the clobber to complete, we could save
developers' time by renaming the directory and then deleting it in parallel
with the start of the build.

Comment 1

[Adam Roach [:abr]]

Attachment: Perform clobbers in the background on OS X

This patch does two things:

  1. Add a new boolean flag to mozfile.rmtree() that specifies that it is okay to run in the background.

  2. Uses this boolean flag when clobbering the tree to speed up clobbers.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 765583 [details] [diff] [review]
Perform clobbers in the background on OS X

Review of attachment 765583 [details] [diff] [review]:
-----------------------------------------------------------------

I like the idea (I think) but the implementation needs some work.

First, mozfile is part of mozbase and thus has its own code review/landing procedure. More info at https://wiki.mozilla.org/Auto-tools/Projects/MozBase

Second, I'm not sure the implementation of background delete is technically sound. We generally shy away from os.fork() because it doesn't work on Windows. I would suggest just spinning up a new Python thread to perform the rmtree(). However, that would prevent clobber.py from exiting and there wouldn't be a gain. Also, I thought you needed to create a grandchild process, muck with file descriptors, make process a session leader, etc in order for a forked child to truly detach from the parent. Maybe Python is doing this for you? I know the code as written wouldn't work in C.

Then we have the question on whether this is a good idea. I like the idea, but I'm not convinced it's a good idea. I can think of a number of areas where people would object. The biggest IMO is the potential for a detached process to linger after the build has completed. This would not be good for our automation infrastructure. So, I think if we had this, we'd need to ensure the background process/thread exits with the main build process. And, we may want to make it configurable.

Let's think about this so more.

Comment 3

[Adam Roach [:abr]]

(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 765583 [details] [diff] [review]


> Second, I'm not sure the implementation of background delete is technically
> sound. We generally shy away from os.fork() because it doesn't work on
> Windows.

Yeah, that's the explicit reason I'm checking for Darwin instead of letting it happen on all platforms. I think there are some bad interactions with some of the BSD systems as well, IIRC. The approach probably works okay on Linux, but the gain is minimal, so I figured it wasn't worth the extra testing.

> Also, I thought you needed to create a grandchild
> process, muck with file descriptors, make process a session leader, etc in
> order for a forked child to truly detach from the parent.

You're right -- this process will zombie until the parent exits. It's hard to see the harm of short-lived zombies for a build system, but I'm happy to clean it up for hygiene reasons. FWIW, the python approach for doing this "correctly" is almost exactly how you'd do it in C: http://code.activestate.com/recipes/278731/

> Then we have the question on whether this is a good idea. I like the idea,
> but I'm not convinced it's a good idea. I can think of a number of areas
> where people would object. The biggest IMO is the potential for a detached
> process to linger after the build has completed. This would not be good for
> our automation infrastructure.

While clobbers take a "long time," I don't think they're going to exceed the amount of time spent building the tree, even for an incremental build. But I do understand that changes to automation can be fidgety, and would be perfectly happy disabling this behavior on the builders.

> So, I think if we had this, we'd need to
> ensure the background process/thread exits with the main build process. And,
> we may want to make it configurable.

Sure, we could make it so that users must set an environment variable to activate the behavior (I don't think we want the tree deletion to abort if it happens to outlive the build, though). Of course, that would save far fewer people's time, since there's a publicity issue involved in getting people to turn it on, but it seems a reasonable compromise to avoid disrupting delicate workflows (cf https://xkcd.com/1172/).


> Let's think about this so more.

Sure. Should I open a discussion on dev-platform? Is there a mailing list for MozBase? or do I just need to jump into #ateam and hope the right people are on and free to engage on the topic?

Comment 4

[Adam Roach [:abr]]

I'm not sure when it happened, but OS X clobbers are oodles faster now than they were when I opened this bug. I don't believe that worth the effort to try to fix any longer.

> $ time ./mach clobber
> 
> real	0m10.497s
> user	0m1.782s
> sys	0m4.364s