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

refactor(cache): simplify noCache condition #362

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

agilgur5
Copy link
Collaborator

Summary

Greatly simplify the noCache condition in tscache -- basically most of the code is not used and can be removed or can early return in the case that there is no cache.

Details

  • if there is no cache, we don't need to do any operations on a cache at all, so we can just totally skip all the cache operations and return early

    • this should be a perf improvement, at the very least on memory usage, as lots of stuff isn't created or processed now
      • this may make clean: true a more optimal choice for smaller projects, as the FS usage when writing to cache may be slower than the small amount of compilation and type-checking required in small projects
  • to start, from the constructor, the only necessary piece when noCache is the dependency tree

    • this is needed for walkTree and setDependency to function, but otherwise is unused when noCache
      • this might be able to be further optimized to remove the graph entirely when noCache, but that is saved for potential future work and not part of this PR
    • so, we can just move the tree creation further up in the constructor as its previous ordering within the constructor does not actually matter
    • once this is done, we can just early return when noCache instead of doing all the cache-related actions, since they're not needed when there is no cache
      • no need to set cacheDir or any hashes etc since they're not used
        • note that clean only uses cachePrefix and cacheRoot, which are already set, and does not use cacheDir
      • no need to init the cache as it's not used
        • also slightly change the ordering to move init right after its prereqs are done, i.e. setting cacheDir, hashOptions, etc
          • just keeps with the flow instead of calling it in the middle of the ambient type processing
      • no need to check ambient types as that is only used for cache invalidation (marking things dirty), which is not used when there is no cache
        • note that isDirty is literally never called when noCache
  • from there, since we don't call checkAmbientTypes or init when noCache (the constructor is the only place they are called and they are both private), we can entirely remove their noCache branches

    • fairly simple for checkAmbientTypes, we just remove the tiny if block that sets ambientTypesDirty, as, well, "dirty" isn't used when there is no cache
    • for init, this means we can entirely remove the creation of NoCache, which isn't needed when there is no cache
      • that means we can also remove the implementation and tests for NoCache
        • and the reference to it in CONTRIBUTING.md
  • in done (which is public), we can also simply skip rolling caches and early return when there is no cache

  • the only other tiny change is the non-null assertions for ambientTypes and cacheDir

    • this matches the existing, simplifying non-null assertions for all the caches, so I did not workaround that
      • could set a default of an empty array for ambientTypes etc to workaround this, but thought it better to match existing code style and not add new things
    • this also matches the behavior, as while ambientTypes and cacheDir could be null, this is only if there is no cache, in which case, they are never used

Potential Future Work

  • Further optimize noCache by exploring if the whole dependencyTree could be removed in that case
  • Potentially add heuristics for clean, as it actually may be more performant in the case of smaller projects
    • This would need some perf testing as well

- if there is no cache, we don't need to do any operations on a cache at all, so we can just totally skip all the cache operations and return early
  - this should be a perf improvement, at the very least on memory usage, as lots of stuff isn't created or processed now
    - this may make `clean: true` a more optimal choice for smaller projects, as the FS usage when writing to cache may be slower than the small amount of compilation and type-checking required in small projects

- to start, from the constructor, the only necessary piece when `noCache` is the dependency tree
  - this is needed for `walkTree` and `setDependency` to function, but otherwise is unused when `noCache`
    - this _might_ be able to be further optimized to remove the graph entirely when `noCache`, but that is saved for potential future work and not part of this commit
  - so, we can just move the tree creation further up in the constructor as its previous ordering within the constructor does not actually matter
  - once this is done, we can just early return when `noCache` instead of doing all the cache-related actions, since they're not neeeded when there is no cache
    - no need to set `cacheDir` or any hashes etc since they're not used
      - note that `clean` only uses `cachePrefix` and `cacheRoot`, which are already set, and does not use `cacheDir`
    - no need to `init` the cache as it's not used
      - also slightly change the ordering to move `init` right after its prereqs are done, i.e. setting `cacheDir`, `hashOptions`, etc
        - just keeps with the flow instead of calling it in the middle of the ambient type processing
    - no need to check ambient types as that is only used for cache invalidation (marking things dirty), which is not used when there is no cache
      - note that `isDirty` is literally never called when `noCache`

- from there, since we don't call `checkAmbientTypes` or `init` when `noCache` (the constructor is the only place they are called and they are both `private`), we can entirely remove their `noCache` branches
  - fairly simple for `checkAmbientTypes`, we just remove the tiny if block that sets `ambientTypesDirty`, as, well, "dirty" isn't used when there is no cache
  - for `init`, this means we can entirely remove the creation of `NoCache`, which isn't needed when there is no cache
    - that means we can also remove the implementation and tests for `NoCache`
      - and the reference to it in `CONTRIBUTING.md`

- in `done`, we can also simply skip rolling caches and early return when there is no cache

- the only other tiny change is the non-null assertions for `ambientTypes` and `cacheDir`
  - this matches the existing, simplifying non-null assertions for all the caches, so I did not workaround that
    - _could_ set a default of an empty array for `ambientTypes` etc to workaround this, but thought it better to match existing code style and not add new things
  - this also matches the behavior, as while `ambientTypes` and `cacheDir` could be `null`, this is only if there is no cache, in which case, they are never used
@agilgur5 agilgur5 added kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: cache Related to the cache labels Jun 21, 2022
@ezolenko ezolenko merged commit 67f1d86 into ezolenko:master Jun 24, 2022
@agilgur5 agilgur5 deleted the refactor-cache-simplify-nocache branch July 2, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: cache Related to the cache
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants