Prefer zstd over gzip #270

aiqiaoy · 2020-04-23T19:07:38Z

Fixes #184

A lot of the work was completed by @imbsky in PR #202. Additionally, this PR adds:

zstd in the cache version
A gzip fallback path for self-hosted runners that do not have zstd installed
Jest tests

The text was updated successfully, but these errors were encountered:

smorimoto · 2020-04-23T22:28:33Z

Awesome @aiqiaoy!! I will review this PR soon 🙂 Thanks for your work!

smorimoto · 2020-04-23T23:10:47Z

Perhaps you should pick this commit (c7743bc) change to make the test work correctly. (zstd is not installed by default in ubuntu:latest, which is currently used for proxy-related testing. So, for testing, I just created an image of ubuntu:latest with zstd, so we can use it to test.)

thomaseizinger · 2020-04-27T02:51:59Z

src/cacheHttpClient.ts

@@ -82,12 +83,11 @@ function createHttpClient(): HttpClient {
    );
 }

-export function getCacheVersion(): string {
+export function getCacheVersion(useZstd?: boolean): string {


This function is a "boolean-trap". It might be worth considering using an enum here, like:

enum CompressionTool { Zstd, GnuTar }

Well, in my opinion, this is not a big deal. The enum has mistakes, depending on how you look at it, and should not be actively recommended for use. If you want to avoid use boolean, you should use string literal types combine with union types. But it's too overkill for this purpose. So, there is no problem with the current code.

Never said that it is a big deal :)

I was more coming from a readability-PoV because the first change I saw in the PR diff was:

test("getCacheVersion with zstd compression returns version", async () => { testUtils.setInput(Inputs.Path, "node_modules"); const result = getCacheVersion(true); expect(result).toEqual( "273877e14fd65d270b87a198edbfa2db5a43de567c9a548d2a2505b408befe24" ); });

and I didn't understand what getCacheVersion(true) means, hence the comment about boolean-traps :)

Sorry. This kind of expression is really common in Japanese, So I said. (English is still difficult for me...) It certainly seems to have a readability problem, but I think it's basically within the acceptable range.

If this needs to be changed, I suggest a major redesign, but it requires too much work and this action probably won't be a large codebase that it needs.

PR updated to use enum instead.

.github/workflows/workflow.yml

smorimoto · 2020-04-27T20:21:21Z

@aiqiaoy This should work.

const args = [
  ...(useZstd ? ["--use-compress-program", "zstd -T0 --long=31"] : ["-z"]),
  "-xf",
  archivePath.replace(new RegExp("\\" + path.sep, "g"), "/"),
  "-P",
  "-C",
  workingDirectory.replace(new RegExp("\\" + path.sep, "g"), "/"),
];

smorimoto · 2020-04-27T20:22:20Z

This isn't a tar problem, it's a strange specification of Node.js.

aiqiaoy · 2020-04-28T17:00:18Z

@aiqiaoy This should work.

const args = [
  ...(useZstd ? ["--use-compress-program", "zstd -T0 --long=31"] : ["-z"]),
  "-xf",
  archivePath.replace(new RegExp("\\" + path.sep, "g"), "/"),
  "-P",
  "-C",
  workingDirectory.replace(new RegExp("\\" + path.sep, "g"), "/"),
];

@imbsky Thanks! Nodejs newbie here and you definitely saved me a lot of headaches lol

smorimoto · 2020-04-28T17:02:37Z

My pleasure. I had the same experience before when implementing this hehe...

smorimoto · 2020-04-28T17:19:24Z

The format check seems to have failed.

smorimoto · 2020-04-29T02:47:14Z

Oh, all tests have passed now. I will review this soon. Perhaps you are also interested in doing that. @joshmgross

joshmgross

Looks good overall! Left some clarifying comments

src/cacheHttpClient.ts

src/tar.ts

src/utils/actionUtils.ts

smorimoto · 2020-04-29T16:00:34Z

All of the following flags are needed for compression,

zstd -T0 --long=31

and all of the following flags are needed for a decompression.

zstd --long=31 -d

CC: @kcgen

smorimoto · 2020-04-29T16:01:47Z

This explanation by @kcgen is really impressive.
#184 (comment)

aiqiaoy · 2020-04-29T18:35:11Z

All of the following flags are needed for compression,
zstd -T0 --long=31
and all of the following flags are needed for a decompression.
zstd --long=31 -d

The doc says the maximum for --long is 30 (1 GiB) on 32-bit platforms and 31 (2 GiB) on 64-bit platforms.

@joshmgross Do you know if we support 32 bits OS for our self-hosted runners?

joshmgross · 2020-04-30T14:20:13Z

@joshmgross Do you know if we support 32 bits OS for our self-hosted runners?

Yep we do. We at least have an ARM32 Runner that's in Pre-Release

Add zstd to cache versioning

aiqiaoy · 2020-05-01T14:10:18Z

PR updated.

@joshmgross @imbsky It would be great if you can take a look again when you have time :)

joshmgross

Awesome! 🚢

src/tar.ts

smorimoto · 2020-05-01T15:26:45Z

This looks good to me! I really appreciate your work!

…d 64-bit OS

aiqiaoy · 2020-05-01T18:31:16Z

Thanks everyone for the review and feedback. I'll check this PR in early Monday just to be on the safer side.

smorimoto

Looks good to me!

smorimoto · 2020-05-04T14:52:59Z

Thanks @aiqiaoy for doing the rest of the work.
Thanks @miketimofeev for helping me make changes to the infrastructure.
and kudos to @kcgen for helping me most with this change.
Thank you all!

shunkakinoki · 2020-05-04T15:47:55Z

It is way faster, thank you!

smorimoto · 2020-05-04T16:09:07Z

@joshmgross Is there anything left to do for the v2 or next v1 series release?

smorimoto · 2020-05-04T16:10:22Z

If I remember correctly, the multi path stuff and this were the only remaining tasks for v2.

joshmgross · 2020-05-04T17:03:46Z

@joshmgross Is there anything left to do for the v2 or next v1 series release?

We're tracking #263 for v2, but that might be back-ported to v1 as well.

smorimoto · 2020-05-04T19:13:24Z

Oh, I see. Will the next version probably be released after the PR is merged?

smorimoto · 2020-05-04T19:15:29Z

Well, I just want to check, so there is no rush to do anything.

thomaseizinger reviewed Apr 27, 2020

View changes

joshmgross reviewed Apr 27, 2020

View changes

.github/workflows/workflow.yml Outdated Show resolved Hide resolved

aiqiaoy marked this pull request as draft Apr 27, 2020

aiqiaoy force-pushed the users/aiyan/zstd branch from c69540a to 619a527 Apr 27, 2020

aiqiaoy marked this pull request as ready for review Apr 28, 2020

joshmgross reviewed Apr 29, 2020

View changes

src/cacheHttpClient.ts Outdated Show resolved Hide resolved

src/tar.ts Outdated Show resolved Hide resolved

src/tar.ts Outdated Show resolved Hide resolved

src/utils/actionUtils.ts Outdated Show resolved Hide resolved

Use zstd instead of gzip if available

97f7baa

Add zstd to cache versioning

aiqiaoy force-pushed the users/aiyan/zstd branch 2 times, most recently from 8e1ec33 to 76bba55 Apr 30, 2020

Address PR feedback

a5d9a3b

aiqiaoy force-pushed the users/aiyan/zstd branch from 76bba55 to a5d9a3b May 1, 2020

joshmgross approved these changes May 1, 2020

View changes

src/tar.ts Outdated Show resolved Hide resolved

smorimoto approved these changes May 1, 2020

View changes

Use 30 as the long distance matching window to support both 32-bit an…

75cd46e

…d 64-bit OS

smorimoto approved these changes May 1, 2020

View changes

aiqiaoy deleted the users/aiyan/zstd branch May 13, 2020

richardxia mentioned this pull request May 16, 2020

Incorrect parameters passed to zstd #316

Closed

dhadka mentioned this pull request Oct 20, 2020

Decompression of cache is slow - 850MB take 49s #435

Open

felixhandte mentioned this pull request Jan 20, 2021

Add GitHub Actions to "Zstandard is used by" list facebook/zstd#2477

Closed

actions / cache Public

Prefer zstd over gzip #270

Prefer zstd over gzip #270

aiqiaoy commented Apr 23, 2020

smorimoto commented Apr 23, 2020

smorimoto commented Apr 23, 2020 •

edited

thomaseizinger Apr 27, 2020

smorimoto Apr 27, 2020

thomaseizinger Apr 28, 2020

smorimoto Apr 28, 2020

smorimoto Apr 28, 2020

aiqiaoy Apr 28, 2020

smorimoto commented Apr 27, 2020

smorimoto commented Apr 27, 2020

aiqiaoy commented Apr 28, 2020

smorimoto commented Apr 28, 2020

smorimoto commented Apr 28, 2020

smorimoto commented Apr 29, 2020

joshmgross left a comment

smorimoto commented Apr 29, 2020

smorimoto commented Apr 29, 2020

aiqiaoy commented Apr 29, 2020 •

edited

joshmgross commented Apr 30, 2020

aiqiaoy commented May 1, 2020

joshmgross left a comment

smorimoto commented May 1, 2020

aiqiaoy commented May 1, 2020

smorimoto left a comment

smorimoto commented May 4, 2020

shunkakinoki commented May 4, 2020

smorimoto commented May 4, 2020

smorimoto commented May 4, 2020 •

edited

joshmgross commented May 4, 2020

smorimoto commented May 4, 2020

smorimoto commented May 4, 2020

actions / cache Public

Prefer zstd over gzip #270

Prefer zstd over gzip #270

Conversation

aiqiaoy commented Apr 23, 2020

smorimoto commented Apr 23, 2020

smorimoto commented Apr 23, 2020 • edited

thomaseizinger Apr 27, 2020

smorimoto Apr 27, 2020

thomaseizinger Apr 28, 2020

smorimoto Apr 28, 2020

smorimoto Apr 28, 2020

aiqiaoy Apr 28, 2020

smorimoto commented Apr 27, 2020

smorimoto commented Apr 27, 2020

aiqiaoy commented Apr 28, 2020

smorimoto commented Apr 28, 2020

smorimoto commented Apr 28, 2020

smorimoto commented Apr 29, 2020

joshmgross left a comment

smorimoto commented Apr 29, 2020

smorimoto commented Apr 29, 2020

aiqiaoy commented Apr 29, 2020 • edited

joshmgross commented Apr 30, 2020

aiqiaoy commented May 1, 2020

joshmgross left a comment

smorimoto commented May 1, 2020

aiqiaoy commented May 1, 2020

smorimoto left a comment

smorimoto commented May 4, 2020

shunkakinoki commented May 4, 2020

smorimoto commented May 4, 2020

smorimoto commented May 4, 2020 • edited

joshmgross commented May 4, 2020

smorimoto commented May 4, 2020

smorimoto commented May 4, 2020

smorimoto commented Apr 23, 2020 •

edited

aiqiaoy commented Apr 29, 2020 •

edited

smorimoto commented May 4, 2020 •

edited