Skip to content
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

Replace subprocess::Popen() with boost::process #398

Merged

Conversation

ChrisCummins
Copy link
Contributor

@ChrisCummins ChrisCummins commented Sep 15, 2021

This patch set:

message Command {
  // A list of command line arguments.
  repeated string argument = 1;
  // An optional key-value mapping of environment variables to set.
  map<string, string> env = 2;
  // The maximum runtime of the command.
  int32 timeout_seconds = 3;
  // An optional list of files that are required by this command. If set, the
  // presence of the files will be tested for before running the command. This
  // can be useful for providing informative error messages.
  repeated string infile = 4;
  // An optional list of files that are generated by this command. If set, the
  // presence of the files will be tested for after running the command.
  repeated string outfile = 5;
}

Fixes #393.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 15, 2021
@ChrisCummins ChrisCummins force-pushed the fix-llvm-runtime-slowdown branch from 2a8c741 to e3d8ee6 Compare September 15, 2021 20:46
@ChrisCummins ChrisCummins added this to the v0.1.11 milestone Sep 15, 2021
@ChrisCummins ChrisCummins force-pushed the fix-llvm-runtime-slowdown branch from ee1bf91 to e3ccc75 Compare September 16, 2021 08:16
@ChrisCummins ChrisCummins marked this pull request as ready for review September 16, 2021 08:18
@ChrisCummins ChrisCummins changed the title 🏗️ WIP: Replace subprocess::Popen() with boost::process Replace subprocess::Popen() with boost::process Sep 16, 2021
@ChrisCummins ChrisCummins force-pushed the fix-llvm-runtime-slowdown branch from 7955423 to 5baf885 Compare September 16, 2021 08:30
@ChrisCummins ChrisCummins force-pushed the fix-llvm-runtime-slowdown branch from 7818118 to 2c636d7 Compare September 17, 2021 08:59
ChrisCummins and others added 13 commits September 17, 2021 10:40
The subprocess::Popen() library function was found to slow down when
called in a tight loop thousands of times. This patch reimplements the
compiler_gym::util::checkOutput() function to use boost::process
rather than subprocess::Popen.

On a test bench, the runtimes of this new implementation have been
found to be stable after ~50k runs.

Fixes facebookresearch#393.
This is similar to checkCall() except that it copies the stdout to a
string output parameter.
This creates a new Command protocol buffer that encapsulates all of
the details for running a single command. The Subprocess.h header has
been updated with a new class to use this proto buffer.
The coverage workflow has been integrated into the new CI workflow.
This is a clumsy workaround that simply disables the timeout check on
macOS, so that RPC timeouts are required to enforce timeouts.

Issue facebookresearch#399.
@ChrisCummins ChrisCummins force-pushed the fix-llvm-runtime-slowdown branch from 2c636d7 to 935b8cc Compare September 17, 2021 09:41
@ChrisCummins ChrisCummins merged commit 0b7c44c into facebookresearch:development Sep 17, 2021
@ChrisCummins ChrisCummins deleted the fix-llvm-runtime-slowdown branch September 28, 2021 14:40
This was referenced Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Taking Runtime observation repeatedly gets slower over time
2 participants