Only rewrite unit files when outdated #32

xianwen · 2019-09-14T08:51:15Z

Compare the timestamps of target Unit file and the source Unit file, and skip the Unit remapping and writing if the target Unit file is newer than the source Unit file.

This PR addresses #6

…t file

kastiglione

Thanks @xianwen! I have a couple inline comments.

Can you format with clang-format?

kastiglione · 2019-09-19T18:47:18Z

index-import.cpp

+                           StringRef inputFile,
+                           IndexUnitWriter &writer) {
+  std::string error;
+  auto IsUptodateOpt = writer.isUnitUpToDateForOutputFile(outputFile, inputFile, error);


In this file/project variables start with a lower case letter.

kastiglione · 2019-09-19T18:49:40Z

index-import.cpp

+
+  // Check if the unit file is already up to date
+  SmallString<256> outputFileFullPath;
+  path::append(outputFileFullPath, workingDir, outputFile);


Is outputFile guaranteed to be relative? I think it can be absolute, I'll have to check.

In our case, we'd want to use the output file path before it gets remapped. For our builds, we don't copy the .o files, but we remap because that's what Xcode uses to lookup records in the index.

Maybe we should check both the path before and after remapping?

@kastiglione, thanks for your comment!

Is outputFile guaranteed to be relative? I think it can be absolute, I'll have to check.

You're right, it can be absolute. I'll update this part.

For our builds, we don't copy the .o files, but we remap because that's what Xcode uses to lookup records in the index.

We need to use the remapped output file path even though the file wouldn't exist. This outputFileFullPath here is only used by isUnitUpToDate to calculate the Unit file name and therefore determine whether the Unit file is already up to date. Even though the ".o" file doesn't exist, we still need to calculate the Unit file path based on the remapped path. Because as you mentioned, that's what Xcode uses to lookup the Unit file.

I'll rename outputFileFullPath to remappedOutputFilePath to make it clearer.

Got it, thanks. I misinterpreted this logic. Carry on :)

kastiglione · 2019-12-19T15:21:32Z

I think I have a need for this now. @xianwen do you plan to pick this back up any time soon?

xianwen · 2020-01-02T19:47:31Z

Hi, @kastiglione, happy new year!
Yes, I'll take a stab at addressing this these few weeks.

xianwen · 2020-01-07T00:57:56Z

Hi, @kastiglione:
All comments are addressed. PTAL when you got a chance.
Thanks!

kastiglione · 2020-01-07T01:00:44Z

@xianwen looks like you'll need to rebase before we can merge.

kastiglione · 2020-01-07T01:00:55Z

Thanks for coming back to it.

index-import.cpp

# Conflicts: # index-import.cpp

xianwen · 2020-01-07T01:18:30Z

@kastiglione, merge conflict resolved :)

kastiglione · 2020-01-07T05:42:46Z

I'll try this out on Wednesday.

kastiglione · 2020-04-03T15:43:27Z

ok, looking at this today!

kastiglione · 2020-04-04T06:41:08Z

I believe this may not help all use cases, particularly for Bazel and Buck. It depends how you integrate with Xcode.

Since the timestamp check is based on the outputFile, which is the path to the Xcode object file. This is where Xcode puts the object file that corresponds to any given source file. However, Bazel (and I presume Buck) do not write their object files into DerivedData.

For this to work, external build system integrations would have to copy object files from their build directories, into Xcode's build directory (ie DerivedData).

What do you think about instead comparing the time stamps of the input unit files, to the outputs. If the input unit file is not newer than an existing file at the output path, then we could skip writing reading->remapping->writing that unit file. This would improve performance for incremental imports.

kastiglione · 2020-04-04T06:41:44Z

also very sorry this took me forever to reply too, I love the idea

brentleyjones · 2020-04-30T17:51:11Z

Since the timestamp check is based on the outputFile, which is the path to the Xcode object file. This is where Xcode puts the object file that corresponds to any given source file. However, Bazel (and I presume Buck) do not write their object files into DerivedData.

The timestamp check is actually just based on the two unit files. The output filename is only used to generate the destination unit filename. The timestamp check is based on the two unit files:

https://github.com/apple/swift-clang/blob/b4c4d68d76e1432032a930efade53a406e1a3c70/lib/Index/IndexUnitWriter.cpp#L219-L253

I ran this locally and it reduced a 22 second import to a 10 second import.

kastiglione · 2020-04-30T18:45:33Z

thanks for clarifying that @brentleyjones. I misinterpreted that API, the name alone sounds like the object file is involved.

brentleyjones · 2020-04-30T18:51:55Z

Inlining the check, and not creating a IndexUnitWriter, reduced the time down to 5 seconds for me. About 2 seconds for cloning records and 3 for checking timestamps.

kastiglione · 2020-04-30T18:56:57Z

that's great, when you showed the numbers my instinct was that it could be optimized to avoid more work

kastiglione · 2020-04-30T18:58:05Z

@DavidGoldman has also talked parallelizing along unit files instead of along indexstores. That would also help the perf.

kastiglione · 2020-04-30T18:58:48Z

seems like it's safe to merge this then

segiddins · 2020-06-12T18:43:34Z

@kastiglione thoughts on getting this merged / released?

kastiglione · 2020-08-06T23:57:45Z

@segiddins I'm going to review #46 first, I believe it subsumes this change.

xianwen added 2 commits September 14, 2019 01:44

Skip the unit import if the target unit file is newer than source uni…

42b9540

…t file

Use StringRef instead of std::string

5b67392

kastiglione reviewed Sep 19, 2019

View reviewed changes

xianwen added 2 commits January 6, 2020 16:47

CR feedback

e93e485

Rename isUptodateOpt to isUpToDateOpt

2448fb9

kastiglione reviewed Jan 7, 2020

View reviewed changes

index-import.cpp Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into check-unit-up-to-date

58e5312

# Conflicts: # index-import.cpp

brentleyjones mentioned this pull request Jun 15, 2020

Faster incremental imports #46

Closed

brentleyjones mentioned this pull request Jan 22, 2021

Add option to only transfer units if they are newer #52

Merged

brentleyjones closed this in #52 Jan 25, 2021

Only rewrite unit files when outdated #32

Only rewrite unit files when outdated #32

Uh oh!

Conversation

xianwen commented Sep 14, 2019

Uh oh!

kastiglione left a comment

Choose a reason for hiding this comment

Uh oh!

kastiglione Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

xianwen Jan 7, 2020

Choose a reason for hiding this comment

Uh oh!

kastiglione Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

kastiglione Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

xianwen Sep 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xianwen Sep 22, 2019

Choose a reason for hiding this comment

Uh oh!

kastiglione Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kastiglione commented Dec 19, 2019

Uh oh!

xianwen commented Jan 2, 2020

Uh oh!

xianwen commented Jan 7, 2020

Uh oh!

kastiglione commented Jan 7, 2020

Uh oh!

kastiglione commented Jan 7, 2020

Uh oh!

Uh oh!

xianwen commented Jan 7, 2020

Uh oh!

kastiglione commented Jan 7, 2020

Uh oh!

kastiglione commented Apr 3, 2020

Uh oh!

kastiglione commented Apr 4, 2020

Uh oh!

kastiglione commented Apr 4, 2020

Uh oh!

brentleyjones commented Apr 30, 2020

Uh oh!

kastiglione commented Apr 30, 2020

Uh oh!

brentleyjones commented Apr 30, 2020

Uh oh!

kastiglione commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kastiglione commented Apr 30, 2020

Uh oh!

kastiglione commented Apr 30, 2020

Uh oh!

segiddins commented Jun 12, 2020

Uh oh!

kastiglione commented Aug 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xianwen Sep 22, 2019 •

edited

Loading

kastiglione Sep 25, 2019 •

edited

Loading

kastiglione commented Apr 30, 2020 •

edited

Loading