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

state: Track module directory ModTime #989

Closed
6 tasks
radeksimko opened this issue Jul 6, 2022 · 3 comments
Closed
6 tasks

state: Track module directory ModTime #989

radeksimko opened this issue Jul 6, 2022 · 3 comments
Assignees
Labels
enhancement New feature or request modules Functionality related to the module block and modules generally
Milestone

Comments

@radeksimko
Copy link
Member

Background

Currently, whenever we schedule ParseModuleConfiguration, ParseVariables, or ParseModuleManifest from anywhere, we just parse these files without checking whether we have already parsed the exact same file (with the same content) previously.

The scheduler already de-duplicates jobs, such that we never run the same job for the same folder more than once, if it's already in the queue. This is a good barrier, but we anticipate the following features to put more pressure on the scheduler and create more situations of duplicate work (same job for the same folder) scheduled more sparsely, which the scheduler itself has no chance of catching.

To avoid wasting CPU on these unnecessary jobs we need some way of checking what files we have processed and whether they changed since.

The simplest way to do it would be reading, storing and comparing ModTime() (modification time) of the module folder.

Proposal

  • Introduce DirModTime time.Time to state.Module
  • state: Add lastModTime time.Time as a 2nd argument to ModuleStore.Add(modPath string)
  • state: Introduce ModuleStore.HasChangedSince(modPath string, t time.Time) (bool, error)
  • walker: Check ModTime() of every dir and pass it to HasChangedSince() before scheduling any jobs
  • textDocument/didOpen handler: Check ModTime() of every dir and pass it to HasChangedSince() before scheduling any jobs
    // We reparse because the file being opened may not match
    // (originally parsed) content on the disk
    // TODO: Do this only if we can verify the file differs?
    modHandle := document.DirHandleFromPath(mod.Path)
    jobIds, err := svc.indexer.DocumentChanged(modHandle)
    if err != nil {
    return err
    }
  • workspace/didChangeWorkspaceFolders handler: Check ModTime() of every dir and pass it to HasChangedSince() before scheduling any jobs
@radeksimko radeksimko added the enhancement New feature or request label Jul 6, 2022
@radeksimko radeksimko added this to the v0.29.0 milestone Jul 7, 2022
@radeksimko radeksimko self-assigned this Jul 13, 2022
@radeksimko radeksimko added the modules Functionality related to the module block and modules generally label Jul 13, 2022
@radeksimko
Copy link
Member Author

@jpogran raised some valid concerns/questions yesterday:

  1. this may be difficult to implement on Windows as directory modification times are interpreted differently than on Unix
  2. could we not just assume it's always up-to-date and fully rely on the LSP watch mechanism?

I want to try to find answer to (2) in particular as that looks like a solution that would better align with LSP spec, where the client watches for changes and notifies the server.

The only challenge is that we cannot easily treat pure existence of a module (as entry in memdb) as a guarantee that the module has all the data (i.e. that it was parsed, decoded etc.), so we'd likely need to do some deeper checking of the module - e.g. check the *State fields on the Module:

type Module struct {
Path string
ModManifest *datadir.ModuleManifest
ModManifestErr error
ModManifestState op.OpState
TerraformVersion *version.Version
TerraformVersionErr error
TerraformVersionState op.OpState
InstalledProviders InstalledProviders
ProviderSchemaErr error
ProviderSchemaState op.OpState
RefTargets reference.Targets
RefTargetsErr error
RefTargetsState op.OpState
RefOrigins reference.Origins
RefOriginsErr error
RefOriginsState op.OpState
VarsRefOrigins reference.Origins
VarsRefOriginsErr error
VarsRefOriginsState op.OpState
ParsedModuleFiles ast.ModFiles
ParsedVarsFiles ast.VarsFiles
ModuleParsingErr error
VarsParsingErr error
ModuleParsingState op.OpState
VarsParsingState op.OpState
Meta ModuleMetadata
MetaErr error
MetaState op.OpState

@radeksimko
Copy link
Member Author

Closing in favour of #1002

@radeksimko radeksimko closed this as not planned Won't fix, can't repro, duplicate, stale Jul 18, 2022
@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request modules Functionality related to the module block and modules generally
Projects
None yet
Development

No branches or pull requests

1 participant