Skip to content

(Option 1) Improve logging accuracy and reduce exists && isFile to 1 file query #295

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.

Sign up for GitHub

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

Closed
marler8997 wants to merge 1 commit into from
Closed

(Option 1) Improve logging accuracy and reduce exists && isFile to 1 file query #295

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 100 additions & 43 deletions rdmd.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ int main(string[] args)
// Look for the D compiler rdmd invokes automatically in the same directory as rdmd
// and fall back to using the one in your path otherwise.
string compilerPath = buildPath(dirName(thisExePath()), defaultCompiler);
if (compilerPath.exists && compilerPath.isFile)
if (Yap.existsAsFile(compilerPath))
compiler = compilerPath;

if (args.length > 1 && args[1].startsWith("--shebang ", "--shebang="))
Expand Down Expand Up @@ -233,9 +233,7 @@ int main(string[] args)
immutable workDir = getWorkPath(root, compilerFlags);
lockWorkPath(workDir); // will be released by the OS on process exit
string objDir = buildPath(workDir, "objs");
yap("mkdirRecurse ", objDir);
if (!dryRun)
mkdirRecurse(objDir);
Yap.mkdirRecurseIfLive(objDir);

if (lib)
{
Expand Down Expand Up @@ -292,16 +290,14 @@ int main(string[] args)
// Both exe and buildWitness exist, and exe is older than
// buildWitness. This is the only situation in which we
// may NOT need to recompile.
yap("stat ", buildWitness);
lastBuildTime = buildWitness.timeLastModified(SysTime.min);
lastBuildTime = Yap.timeLastModified(buildWitness, SysTime.min);
}
}
else
{
exe = buildPath(workDir, exeBasename) ~ outExt;
buildWitness = exe;
yap("stat ", buildWitness);
lastBuildTime = buildWitness.timeLastModified(SysTime.min);
lastBuildTime = Yap.timeLastModified(buildWitness, SysTime.min);
}

// Have at it
Expand Down Expand Up @@ -395,9 +391,7 @@ private @property string myOwnTmpDir()
else
tmpRoot = tmpRoot.replace("/", dirSeparator).buildPath(".rdmd");

yap("mkdirRecurse ", tmpRoot);
if (!dryRun)
mkdirRecurse(tmpRoot);
Yap.mkdirRecurseIfLive(tmpRoot);
return tmpRoot;
}

Expand Down Expand Up @@ -427,9 +421,7 @@ private string getWorkPath(in string root, in string[] compilerFlags)
workPath = buildPath(tmpRoot,
"rdmd-" ~ baseName(root) ~ '-' ~ hash);

yap("mkdirRecurse ", workPath);
if (!dryRun)
mkdirRecurse(workPath);
Yap.mkdirRecurseIfLive(workPath);

return workPath;
}
Expand Down Expand Up @@ -466,10 +458,10 @@ private int rebuild(string root, string fullExe,
fullExe = fullExe.defaultExtension(".exe");

// Delete the old executable before we start building.
yap("stat ", fullExe);
Copy link
Member

@DmitryOlshansky DmitryOlshansky Jan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yap was a precedent already

Copy link
Contributor

@WebDrake WebDrake Jan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yap appears to be a utility function to report commands issued directly by rdmd. It's responsible for verbose output. The name makes sense for this usage (see the dictionary definition:-), albeit maybe not so obvious to a non-native speaker.

It just seems odd (and misleading) to use as a prefix for the newly introduced functions.

if (exists(fullExe))
auto fullExeAttributes = Yap.getFileAttributes(fullExe);
if (fullExeAttributes.exists)
{
enforce(!isDir(fullExe), fullExe ~ " is a directory");
enforce(fullExeAttributes.isFile, fullExe ~ " is not a regular file");
yap("rm ", fullExe);
if (!dryRun)
{
Expand All @@ -481,8 +473,7 @@ private int rebuild(string root, string fullExe,
// Although we can't delete the file, we can still rename it.
auto oldExe = "%s.%s-%s.old".format(fullExe,
Clock.currTime.stdTime, thisProcessID);
yap("mv ", fullExe, " ", oldExe);
rename(fullExe, oldExe);
Yap.rename(fullExe, oldExe);
}
}
}
Expand Down Expand Up @@ -530,25 +521,23 @@ private int rebuild(string root, string fullExe,
if (result)
{
// build failed
if (exists(fullExeTemp))
remove(fullExeTemp);
if (Yap.exists(fullExeTemp))
Yap.remove(fullExeTemp);

return result;
}
// clean up the dir containing the object file, just not in dry
// run mode because we haven't created any!
if (!dryRun)
{
yap("stat ", objDir);
if (objDir.exists && objDir.startsWith(workDir))
if (Yap.exists(objDir) && objDir.startsWith(workDir))
{
yap("rmdirRecurse ", objDir);
// We swallow the exception because of a potential race: two
// concurrently-running scripts may attempt to remove this
// directory. One will fail.
collectException(rmdirRecurse(objDir));
collectException(Yap.rmdirRecurse(objDir));
}
yap("mv ", fullExeTemp, " ", fullExe);
rename(fullExeTemp, fullExe);
Yap.rename(fullExeTemp, fullExe);
}
return 0;
}
Expand Down Expand Up @@ -624,7 +613,7 @@ private string[string] getDependencies(string rootModule, string workDir,
foreach (name; names)
{
auto path = buildPath(dir, name);
if (path.exists)
if (Yap.exists(path))
return absolutePath(path);
}
return null;
Expand Down Expand Up @@ -662,8 +651,7 @@ private string[string] getDependencies(string rootModule, string workDir,
auto confFile = captures[2].strip;
// The config file is special: if missing, that's fine too. So
// add it as a dependency only if it actually exists.
yap("stat ", confFile);
if (confFile.exists)
if (Yap.exists(confFile))
{
result[confFile] = null;
}
Expand Down Expand Up @@ -691,8 +679,7 @@ private string[string] getDependencies(string rootModule, string workDir,
// Check if the old dependency file is fine
if (!force)
{
yap("stat ", depsFilename);
auto depsT = depsFilename.timeLastModified(SysTime.min);
auto depsT = Yap.timeLastModified(depsFilename, SysTime.min);
if (depsT > SysTime.min)
{
// See if the deps file is still in good shape
Expand Down Expand Up @@ -740,8 +727,7 @@ private string[string] getDependencies(string rootModule, string workDir,
// Is any file newer than the given file?
bool anyNewerThan(T)(T files, in string file)
{
yap("stat ", file);
return files.anyNewerThan(file.timeLastModified);
return files.anyNewerThan(Yap.timeLastModified(file));
}

// Is any file newer than the given file?
Expand All @@ -767,17 +753,15 @@ than target's. Otherwise, returns true.
private bool newerThan(string source, string target)
{
if (force) return true;
yap("stat ", target);
return source.newerThan(target.timeLastModified(SysTime.min));
return source.newerThan(Yap.timeLastModified(target, SysTime.min));
}

private bool newerThan(string source, SysTime target)
{
if (force) return true;
try
{
yap("stat ", source);
return DirEntry(source).timeLastModified > target;
return Yap.timeLastModified(DirEntry(source)) > target;
}
catch (Exception)
{
Expand Down Expand Up @@ -845,7 +829,7 @@ string makeEvalFile(string todo)
auto srcfile = buildPath(pathname,
"eval." ~ todo.md5Of.toHexString ~ ".d");

if (force || !exists(srcfile))
if (force || !Yap.exists(srcfile))
{
std.file.write(srcfile, todo);
}
Expand All @@ -856,8 +840,7 @@ string makeEvalFile(string todo)
yap("dirEntries ", pathname);
foreach (DirEntry d; dirEntries(pathname, SpanMode.shallow))
{
yap("stat ", d.name);
if (d.timeLastModified < cutoff)
if (Yap.timeLastModified(d) < cutoff)
{
collectException(std.file.remove(d.name));
//break; // only one per call so we don't waste time
Expand Down Expand Up @@ -902,8 +885,7 @@ string which(string path)
foreach (extension; extensions)
{
string absPath = buildPath(envPath, path ~ extension);
yap("stat ", absPath);
if (exists(absPath) && isFile(absPath))
if (Yap.existsAsFile(absPath))
return absPath;
}
}
Expand All @@ -916,3 +898,78 @@ void yap(size_t line = __LINE__, T...)(auto ref T stuff)
debug stderr.writeln(line, ": ", stuff);
else stderr.writeln(stuff);
}

struct FileAttributes
Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document what this is used for.

Copy link
Contributor Author

@marler8997 marler8997 Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok (changed in #299)

{
uint attributes;
bool exists() const { return attributes != 0xFFFFFFFF; }
bool isFile() const
in { assert(exists); } do
{
import std.file : attrIsFile;
return attrIsFile(attributes);
}
}

struct Yap
Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yap wasn't a great name start with, and this is not making it better. This is a namespace for wrapping filesystem operations, so let's just name it Filesystem or such.

Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a comment describing the motivation of having this structure.

Copy link
Contributor Author

@marler8997 marler8997 Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment in #299. I like the name Yap for the structure, let's come back to this if you still feel the same after this set of changes.

{
Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move std.file imports here. That way, it will not be possible to accidentally use std.file functions bypassing logging.

Copy link
Contributor Author

@marler8997 marler8997 Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still a fair amount of std.file functions used in the rest of the file

thisExePath
tempDir
remove
FileException
DirEntry
dirEntries

Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use selective imports for that.

remove is sticking out: should it be called via the wrapper instead? If not, then it should be a static or fully-qualified import to make it explicit.

Copy link
Contributor Author

@marler8997 marler8997 Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remove case is awkward. It has a try/catch block with logic to handle what happens if the file is being used. It's in the first part of the rebuild function.

I can switch them to selective imports if you think that's better, but none of the other modules use selective imports, so I wasn't sure if I should follow convention or do the "safer" thing.

Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please use selective imports. Using selective (or static) imports provides half of the justification for the filesystem operations wrapper: to prevent accidentally bypassing it. Without that, it's more difficult to justify this entire pull request.

Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remove call is already accompanied by a yap, so with a bit of refactoring of the !dryRun part it looks like a good candidate of being converted to a call to the wrapper.

Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dirEntries should also probably be moved to the wrapper, since it's not much different from stat in principle.

Copy link
Contributor Author

@marler8997 marler8997 Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done except moving the remove case to a wrapper. have another look at the current set of changes in #299 and let me know what you think

static FileAttributes getFileAttributes(const(char)[] name)
Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use static: to make it more clear that this is a namespace and has no non-static members.

Copy link
Contributor Author

@marler8997 marler8997 Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok (changed in #299)

{
yap("stat ", name);
import std.internal.cstring : tempCString;
version(Windows)
{
import core.sys.windows.windows : GetFileAttributesW;
auto attributes = GetFileAttributesW(name.tempCString!wchar());
return FileAttributes(attributes);
}
else version(Posix)
{
import core.sys.posix.sys.stat : stat_t, stat;
stat_t statbuf = void;
return FileAttributes(
(0 == stat(name.tempCString(), &statbuf)) ? statbuf.st_mode : 0xFFFFFFFF);
}
else static assert(0);
}
static bool exists(const(char)[] name)
{
return Yap.getFileAttributes(name).exists();
}
static bool existsAsFile(const(char)[] name)
{
auto attributes = Yap.getFileAttributes(name);
return attributes.exists() && attributes.isFile();
}
static auto timeLastModified(R)(R name)
{
yap("stat ", name);
return std.file.timeLastModified(name);
Copy link
Member

@CyberShadow CyberShadow Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be written as simply return .timeLastModified(name);, but I guess it's probably better to be more explicit here.

}
static auto timeLastModified(R)(R name, SysTime returnIfMissing)
{
yap("stat ", name);
return std.file.timeLastModified(name, returnIfMissing);
}
static void rename(R,S)(R from, S to)
{
yap("mv ", from, " ", to);
std.file.rename(from, to);
}
static void remove(R)(R name)
{
yap("rm ", name);
std.file.remove(name);
}
static void mkdirRecurseIfLive(const(char)[] name)
{
yap("mkdirRecurse ", name);
if (!dryRun)
std.file.mkdirRecurse(name);
}
static void rmdirRecurse(const(char)[] name)
{
yap("rmdirRecurse ", name);
std.file.rmdirRecurse(name);
}
}