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

fix(emit): eliminate multiple cache files getting out of sync and fix racy emit #13462

Closed
wants to merge 12 commits into from

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Jan 21, 2022

  1. Uses an sqlite database file to store the emit cache to eliminate multiple cache files getting out of sync.
  2. The result of checking the emit is stored in memory and if valid is kept in memory then used in the rest of the emit. This prevents consulting the file system to check if the cache is valid and then reading from the file system later when the cache may no longer be valid, which was racy.

Supersedes #13303.

Closes #13302

WIP though basically done. I could do way more cleanup and have a better design, but wanted to make the minimal changes possible.

TODO:

  • Ensure all calls to get_emit_data verify the source. Maybe an API change would be good to force this (ex. ProcState's SourceMapGetter impl right now retrieves the source map from the cache, but doesn't do any source checks on it). I think though it probably needs more rework in that area and I will have to read the code more.
  • coverage's cover_files does not verify the retrieved emit text is for the source

@dsherret dsherret changed the title fix(emit): use sqlite emit cache to eliminate multiple cache files getting out of sync fix(emit): eliminate multiple cache files getting out of sync and fix racy emit Jan 21, 2022
@stale
Copy link

stale bot commented Mar 26, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 26, 2022
@dsherret dsherret removed the stale label Mar 26, 2022
@jespertheend
Copy link
Contributor

Any updates on this? I'm hoping this will fix #12126

@KnorpelSenf
Copy link
Contributor

Potentially fixes #14206, as well.

@stale
Copy link

stale bot commented Jun 10, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 10, 2022
@bartlomieju bartlomieju removed the stale label Jun 12, 2022
@dsherret
Copy link
Member Author

Closing because this is really out of date. I added an EmitCache trait in #14925 (merged) and will follow up with another PR in a few weeks (before 1.24) that uses the sql cache from this PR (along with doing the same in /x/deno_cache).

@dsherret dsherret closed this Jun 20, 2022
@dsherret dsherret deleted the sqlite_emit_cache branch June 21, 2022 15:07
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.

Emit, source map, and declaration cache in deno dir is racy across multiple processes
5 participants