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

Don't regenerate if input files are older than output file (take 2) #14078

Closed
wants to merge 1 commit into from

Conversation

beyang
Copy link
Member

@beyang beyang commented Sep 23, 2020

First commit is what was pushed in #13782, second commit fixes #14039.

Empirically, this improves time-to-user-readiness for enterprise/dev/start.sh from 40-45s to 32-36s.

@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #14078 into main will decrease coverage by 0.92%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #14078      +/-   ##
==========================================
- Coverage   52.21%   51.28%   -0.93%     
==========================================
  Files        1554     1524      -30     
  Lines       79363    77125    -2238     
  Branches     7085     6903     -182     
==========================================
- Hits        41439    39555    -1884     
+ Misses      34178    33998     -180     
+ Partials     3746     3572     -174     
Flag Coverage Δ
#go 51.63% <ø> (-0.79%) ⬇️
#integration 29.07% <ø> (-1.50%) ⬇️
#storybook 18.21% <ø> (-3.95%) ⬇️
#typescript 50.45% <ø> (-1.25%) ⬇️
#unit 33.86% <ø> (+0.22%) ⬆️
Impacted Files Coverage Δ
cmd/frontend/internal/usagestats/campaigns.go 0.00% <0.00%> (-93.55%) ⬇️
...cker-images/prometheus/cmd/prom-wrapper/silence.go 0.00% <0.00%> (-90.00%) ⬇️
cmd/repo-updater/repos/conf.go 0.00% <0.00%> (-56.25%) ⬇️
...e-intel-bundle-manager/internal/janitor/janitor.go 2.77% <0.00%> (-36.72%) ⬇️
internal/search/types.go 0.00% <0.00%> (-30.44%) ⬇️
enterprise/internal/codeintel/gitserver/client.go 0.00% <0.00%> (-28.29%) ⬇️
internal/db/namespaces.go 63.63% <0.00%> (-24.94%) ⬇️
internal/search/query/labels.go 80.00% <0.00%> (-20.00%) ⬇️
cmd/repo-updater/repos/syncer.go 59.59% <0.00%> (-16.29%) ⬇️
internal/db/default_repos.go 68.96% <0.00%> (-13.39%) ⬇️
... and 1754 more

const generateOperations = {}
for (const outfile of Object.keys(allGenerateOperations)) {
const inputs = allGenerateOperations[outfile].documents
if (watch || (await isInputNewer(inputs, outfile))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we always include everything in watch mode, does this still have any improvement for ./dev/start.sh? Since in ./dev/start.sh we're always watching, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

When dev/start.sh is run, both the non-watch and watch invocation are run in serial. AFAICT the watch run does not actually regenerate until a file has changed (which is why the non-watch invocation is necessary).

Empirically, yes, this shaves at least 10s off for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the watch mode - at least of the GraphQL codegen - actually does run an initial build. If that is correct an even better optimization could actually be to remove the initial, serial, one-time codegen 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in d985b46. Incidentally, this is already done in web/gulpfile.js, so I just copied that.

@@ -29,7 +29,11 @@ const build = gulp.series(generate, webWebpack)
/**
* Watches everything and rebuilds on file changes.
*/
const dev = gulp.parallel(watchGenerate, webWebpackDevServer)
const dev = gulp.series(
Copy link
Member Author

Choose a reason for hiding this comment

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

We want to wait for the initial generate steps to complete before running the webpack dev server for 2 reasons:

  • The generated files may be missing when the the webpack dev server expects them to exist, causing errors to be printed.
  • The generate steps will immediately update the generated files, causing webpack dev server to immediately recompile before its first compilation is complete, wasting CPU cycles for no observable improvement in time-to-ready

@beyang beyang closed this Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants