scons icon indicating copy to clipboard operation
scons copied to clipboard

Improve performance with --duplicate=copy

Open bdbaddog opened this issue 7 years ago • 11 comments

This issue was originally created at: 2010-09-28 13:36:52. This issue was reported by: sherwoodd.

sherwoodd said at 2010-09-28 13:36:52

When --duplicate=copy is used, it appears that the copy is performed every time scons is executes even if the source file hasn't changed.

On our Windows build, this takes up over half of a 5 minute build cycle.

The attached patch adds a new --duplicate mode of copytimestamp. This function only unlinks and then copies the file if the timestamp is different between src and dest.

The other functions and caller have been modified to re-locate the Unlink call so that it only happens when necessary.

sherwoodd said at 2010-09-28 13:37:07

Created an attachment (id=804) Patch

gregnoel said at 2010-11-14 18:09:07

Bug party triage. Nice improvement and a good optimization; should work this way by default. Steven to fix and create followup issue to clean up this code to use an unlink() function in common with other places that need to unlink things (see issue #2693, for example).

garyo said at 2012-09-01 10:01:49

Bumping all old issues targeted for past releases to 2.x.

sherwoodd attached scons_copytimestamp.patch at 2010-09-28 13:37:07.

Patch

bdbaddog avatar Jan 02 '18 14:01 bdbaddog

Guess this went nowhere - what would one do for tests, if this still sounds like a good idea?

mwichmann avatar Apr 04 '22 14:04 mwichmann

It does. So if I'm reading this right, in the case where --duplicate=copy, and the file hasn't changed, it was still copying the file. The only (actual) affect this would have is to update the variant dirs copy of the file's timestamp? So this patch proposes in that case to just copy the timestamp?

I'd take a look at existing --duplicate=copy tests and see if something there could be converted to test fo this?

bdbaddog avatar Apr 04 '22 17:04 bdbaddog

What's supposed to happen if there's an entry for the file in the variantdir? It seemed to me from other evidence (e.g. #2208 and maybe #2934) that SCons doesn't go further to look in the srcdir, but this stuff makes it seem like if duplication is enabled, it's reduplicated in every case (and thus tickling the problem of this report), and maybe only the duplicate=0 case uses the variant-version-before-src-version policy. I guess thinking about it, that would sort of make sense, but trying to confirm before I write some clever-but-incorrect doc updates...

mwichmann avatar Apr 05 '22 23:04 mwichmann

updating my own comment, in #2803 a user suggests:

What's more: With --debug=duplicate, we can see that duplicates are redone on every build

mwichmann avatar Apr 05 '22 23:04 mwichmann

The copying may be noted in the taskmaster trace file.. (think it should be..)

bdbaddog avatar Apr 05 '22 23:04 bdbaddog

So I'm a little confused, there's a lot in play here for something so simple.

  1. Mainly this is going to affect Windows, where the internals refuse to consider the hard-link or soft-link cases, so you always get copies, and then re-copies as in the subject of this issue. Does the Windows platform actually need to exclude hardlinks? Those are doable without issue, but only as long as you use a reasonable filesystem. If you're using FAT/variants, you're presumably on an addon/removable device, would you have to code some sort of fallback to account for that possibility? Or just keep Windows "stupid" and avoid hardlinks?
  2. Is adding new duplication strategies the right way to handle this? Or should it just be an outright change (that's already asked in the old comments on this).
  3. Is (just) checking the timestamp the right way to compare variant vs src version? Or does it need to be something similar to the rest of SCons, where the decider in effect would be used?
  4. Comparing the timestamps as strings seems kind of odd: if (not destexists) or (str(fs.getmtime(src)) != str(fs.getmtime(dest))):

mwichmann avatar Apr 09 '22 23:04 mwichmann

@mwichmann (updated your last comment by changing bullets to numbers so it's easier to discuss)

  1. maybe add as experimental? Then folks can try it, and if there's no issues in the wild, we could enable?
  2. Given our deprecation cycle policy, I'm not sure how making an outright change would work.
  3. What's the downside of the change? You have duplicate=1, and then you locally modify the file (by any means including editor) in the variant dir. Then SCons notices the source dirs copy is different from the variant dir and clobbers the variant dir version. Now in general editing the file copied by the variant_dir logic is BAD (capital letters). But no doubt it happens in the wild. so probably the right thing to do is to create a new --duplicate, roll that out as an option with note that it will become the default, then default it the next major release?
  4. Yes. dubious.. I guess we'd need to use the specified decider to really do that bit correctly.

bdbaddog avatar Apr 10 '22 00:04 bdbaddog

Doesn't the patch for this also resolve #2693 ? Or on windows do we need to make a file writable before we can unlink it?

bdbaddog avatar Apr 10 '22 00:04 bdbaddog

On item 2, there's not a change in documentated behavior. Docs say it makes a copy - without a lot of detail.

By default, SCons physically duplicates the source files and SConscript files as needed into the variant tree.

If possible on the platform, the duplication is performed by linking rather than copying. This behavior is affected by the --duplicate command-line option.

and

There are three ways to duplicate files in a build tree: hard links, soft (symbolic) links and copies. The default policy is to prefer hard links to soft links to copies. You can specify a different policy with this option. ORDER must be one of hard-soft-copy (the default), soft-hard-copy, hard-copy, soft-copy or copy. SCons will attempt to duplicate files using the mechanisms in the specified order.

Doesn't seem to me like a change to not always delete the file before copying it changes anything that's been promised.

mwichmann avatar Apr 10 '22 17:04 mwichmann

Doesn't the patch for this also resolve #2693 ? Or on windows do we need to make a file writable before we can unlink it?

It helps in the sense that the copy doesn't have to be made at all if the file is unchanged, but if there is a change so the variant dir version has to be removed, you still have the permission problem.

mwichmann avatar Apr 10 '22 17:04 mwichmann

Hmmm, the readonly setup doesn't seem to affect the remove-and-copy-to-variantdir, based on simpleminded experiment (as in, it doesn't fail)

mwichmann avatar Apr 10 '22 18:04 mwichmann