Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@arbreng
Copy link
Contributor

@arbreng arbreng commented Jun 10, 2020

PersistentCache no longer manages its version internally. It is the
client's responsibility to inject a version directory at its discretion.

shell/gpu/ no longer depends on shell/common after this, so there is no
need to permute it for product / non-product configurations.

BUG: fxb/52961

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

Moving PersistentCache into flow looks good as a follow-up step of #18938.

However, I'm a little concerned on the change of SetCacheVersion. It seems we didn't call SetCacheVersion anywhere in this PR, or set its default value. Does that mean we're just using an empty string as the version, so upgrading the engine version won't remove the old cache and create a new folder?

It's also unclear why we no longer want to set the engine version as the cache version, and instead let the embedder set it. The fxb/52961 seems to be only about build configs, and there's no mention of persistent cache versions.

@arbreng arbreng marked this pull request as draft June 22, 2020 23:57
@arbreng arbreng requested a review from freiling August 10, 2020 23:23
@arbreng arbreng force-pushed the move-persistent-cache branch from e8391b4 to 567905b Compare August 10, 2020 23:57
@arbreng arbreng marked this pull request as ready for review August 10, 2020 23:58
@arbreng arbreng force-pushed the move-persistent-cache branch from 567905b to e229eb6 Compare August 11, 2020 00:14
@arbreng
Copy link
Contributor Author

arbreng commented Aug 11, 2020

Moving PersistentCache into flow looks good as a follow-up step of #18938.

However, I'm a little concerned on the change of SetCacheVersion. It seems we didn't call SetCacheVersion anywhere in this PR, or set its default value. Does that mean we're just using an empty string as the version, so upgrading the engine version won't remove the old cache and create a new folder?

The latest PR sets the version properly from the Shell now. It was a WIP before.

It's also unclear why we no longer want to set the engine version as the cache version, and instead let the embedder set it. The fxb/52961 seems to be only about build configs, and there's no mention of persistent cache versions.

It's not strictly necessary to have the embedder set the version, but I did that to preserve a good layering between shell/ and flow/ (shell depends on flow, not the other way around). This also allows the unittests to set their own version, and they are not tied to any hard-coded versioning logic like the Shell has.

@arbreng arbreng force-pushed the move-persistent-cache branch 2 times, most recently from 6b4c56c to 8def7a0 Compare August 11, 2020 00:29
@arbreng
Copy link
Contributor Author

arbreng commented Aug 11, 2020

Needs minor cleanup and rebasing on #20422

@arbreng arbreng requested a review from dreveman August 11, 2020 22:13
@arbreng arbreng force-pushed the move-persistent-cache branch 4 times, most recently from ae11936 to 0ff3efe Compare August 12, 2020 05:38
PersistentCache no longer manages its version internally.  It is the
client's responsibility to inject a version directory at its discretion.

shell/gpu/ no longer depends on shell/common after this, so there is no
need to permute it for product / non-product configurations.

BUG: fxb/52961
@arbreng arbreng force-pushed the move-persistent-cache branch from 0ff3efe to f1e07ee Compare August 12, 2020 06:59
@arbreng arbreng marked this pull request as draft August 24, 2020 21:58
@arbreng arbreng closed this Jul 1, 2021
@arbreng arbreng deleted the move-persistent-cache branch July 1, 2021 05:18
@arbreng arbreng restored the move-persistent-cache branch July 1, 2021 05:18
@arbreng arbreng deleted the move-persistent-cache branch October 20, 2021 16:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants