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

Measure local build cache size growth #477

Merged
merged 23 commits into from
Feb 11, 2023
Merged

Measure local build cache size growth #477

merged 23 commits into from
Feb 11, 2023

Conversation

lptr
Copy link
Member

@lptr lptr commented Feb 6, 2023

When tests use the local cache, its size can grow. We want to capture that information in the benchmarks.

@lptr lptr added the @execution label Feb 6, 2023
@lptr lptr self-assigned this Feb 6, 2023
<script src="https://cdnjs.cloudflare.com/ajax/libs/mathjs/7.2.0/math.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/vue/2.7.14/vue.min.js"></script>
<script src='https://cdnjs.cloudflare.com/ajax/libs/Chart.js/3.9.1/chart.min.js'></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/mathjs/11.5.1/math.min.js"></script>

Check warning

Code scanning / CodeQL

Inclusion of functionality from an untrusted source

Script loaded from content delivery network with no integrity check.
<script src='https://cdnjs.cloudflare.com/ajax/libs/Chart.js/3.0.0-beta/chart.min.js'></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/mathjs/7.2.0/math.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/vue/2.7.14/vue.min.js"></script>
<script src='https://cdnjs.cloudflare.com/ajax/libs/Chart.js/3.9.1/chart.min.js'></script>

Check warning

Code scanning / CodeQL

Inclusion of functionality from an untrusted source

Script loaded from content delivery network with no integrity check.
<script src="https://cdnjs.cloudflare.com/ajax/libs/vue/2.6.12/vue.min.js"></script>
<script src='https://cdnjs.cloudflare.com/ajax/libs/Chart.js/3.0.0-beta/chart.min.js'></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/mathjs/7.2.0/math.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/vue/2.7.14/vue.min.js"></script>

Check warning

Code scanning / CodeQL

Inclusion of functionality from an untrusted source

Script loaded from content delivery network with no integrity check.
@lptr lptr changed the title Measure local cache size growth Measure local build cache size growth Feb 7, 2023
Copy link
Member

@asodja asodja left a comment

Choose a reason for hiding this comment

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

LGTM, some comments and tests need attention I see

? Stream.empty()
: Stream.of(cacheDirs);
}
cacheDirectories.forEach(cacheDirectory -> {
Copy link
Member

Choose a reason for hiding this comment

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

💭 Maybe just with streams:

          cacheDirectories
              .map(File::listFiles)
              .filter(Objects::nonNull)
              .flatMap(Stream::of)
              .forEach(file -> {
                  fileCount.incrementAndGet();
                  size.addAndGet(file.length());
              });

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I'll bite :)

import com.google.gson.JsonPrimitive;
import com.google.gson.JsonSerializationContext;
import com.google.gson.JsonSerializer;
import com.google.gson.*;
Copy link
Member

Choose a reason for hiding this comment

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

🧐 Star import that needs attention?

if (cacheFiles == null) {
System.out.println("> Cannot list cache directory " + cacheDir);
} else {
long size = 0;
Copy link
Member

Choose a reason for hiding this comment

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

💭 Maybe this could be:

long size = Stream.of(cacheFiles).mapToLong(File::length).sum()

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@asodja asodja left a comment

Choose a reason for hiding this comment

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

❓ I noticed build cache size is always reported zero, is that just for me?

* Running measured build #25
Execution time 1112 ms
> Build cache size: 0 bytes in 0 file(s) (build-cache-1)
> Build cache size: 0 bytes in 0 file(s) (build-cache-2)

@lptr
Copy link
Member Author

lptr commented Feb 7, 2023

Are you running a build without the build cache being enabled? I get it reported fine:

* Running cleanup for measured build #1
> Build cache size: 17 bytes in 2 file(s) (build-cache-1)
> Build cache size: 48,480,256 bytes in 1 file(s) (build-cache-2)

@lptr lptr requested a review from asodja February 7, 2023 21:01
@lptr lptr marked this pull request as ready for review February 8, 2023 08:04
@lptr
Copy link
Member Author

lptr commented Feb 8, 2023

I added a test for show-build-cache-size.

Copy link
Member

@asodja asodja left a comment

Choose a reason for hiding this comment

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

LGTM


@Override
public double extractValue(T result) {
return extractTotalDurationFrom(result).toNanos() / 1_000_000d;
Copy link
Member

Choose a reason for hiding this comment

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

🧐 Does Duration has .toMillis(), to just do extractTotalDurationFrom(result).toMillis()?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to capture fractional milliseconds.

@lptr
Copy link
Member Author

lptr commented Feb 11, 2023

Well spotted fixes, thanks!

@lptr lptr merged commit 836ce78 into master Feb 11, 2023
@lptr lptr deleted the lptr/measure-cache-size branch February 11, 2023 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants