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 two issues related to using zstd compression #469

Merged
merged 5 commits into from May 19, 2020

Conversation

aiqiaoy
Copy link
Contributor

@aiqiaoy aiqiaoy commented May 18, 2020

Addresses: #462

@aiqiaoy aiqiaoy requested review from dhadka and joshmgross May 18, 2020 20:38
@aiqiaoy aiqiaoy changed the title Users/aiyan/zstd fixes Fix two issues related to using zstd compression May 18, 2020
Copy link
Member

@joshmgross joshmgross left a comment

Choose a reason for hiding this comment

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

Is there a drastic performance when using the --long filter? Would it be simpler to just remove that option from our compression arguments?

packages/cache/src/internal/tar.ts Outdated Show resolved Hide resolved
@@ -5,6 +5,7 @@ export enum CacheFilename {

export enum CompressionMethod {
Gzip = 'gzip',
ZstdOld = 'zstd-old',
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a comment or be more descriptive in the enum name for what's considered an "Old" version of zstd?

The latest version of zstd is v1.4.4, so both of the versions we're dealing with could be considered old.

Comment on lines 92 to 96
return !versionOutput.toLowerCase().includes('zstd command line interface')
? CompressionMethod.Gzip
: !version || semver.lt(version, 'v1.3.2')
? CompressionMethod.ZstdOld
: CompressionMethod.Zstd
Copy link
Member

Choose a reason for hiding this comment

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

This block of quote is a little difficult to parse. It might be better to break this up and add some comments about why we care about v1.3.2

Suggested change
return !versionOutput.toLowerCase().includes('zstd command line interface')
? CompressionMethod.Gzip
: !version || semver.lt(version, 'v1.3.2')
? CompressionMethod.ZstdOld
: CompressionMethod.Zstd
if (!versionOutput.toLowerCase().includes('zstd command line interface')) {
return CompressionMethod.Gzip
}
// v1.3.2 is required to use the `--long` options in ZSTD
if (!version || semver.lt(version, 'v1.3.2') {
return CompressionMethod.ZstdOld
}
return CompressionMethod.Zstd

@aiqiaoy
Copy link
Contributor Author

aiqiaoy commented May 19, 2020

Is there a drastic performance when using the --long filter? Would it be simpler to just remove that option from our compression arguments?

I don’t know much about compression algorithms so I can only say based on @kcgen’s analysis and zstd long range mode release note that the --long filter significantly improves the compression ratio if the files have a lot of matches at a large distance. Compression and decompression speed is also dependent on the amount of long matches.

My understanding is that --long is good for caching for example node modules and different versions of same library but bad for random files.

Copy link
Member

@joshmgross joshmgross left a comment

Choose a reason for hiding this comment

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

Looks good to me!

packages/cache/src/internal/cacheUtils.ts Show resolved Hide resolved
@aiqiaoy aiqiaoy merged commit dcf5c88 into master May 19, 2020

### 0.2.0
- Fixes two issues around using zstd compression algorithm
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Fixes two issues around using zstd compression algorithm
- Fixes issues with the zstd compression algorithm on Windows and Ubuntu 16.04 [#469](https://github.com/actions/toolkit/pull/469)

We might want to link the PRs in the RELEASES notes to help with discoverability

@gitfool
Copy link

gitfool commented May 27, 2020

The latest version of zstd is v1.4.4, so both of the versions we're dealing with could be considered old.

PS. zstd v1.4.5 recently dropped; it's been 6 months since 1.4.4 so probably worth upgrading the virtual environments.

@lazka
Copy link
Contributor

lazka commented Jul 24, 2020

Does anyone here know of an example workflow (or has any hints) on how to make actions/cache use gnu tar and thus zstd on Windows?

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

4 participants