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

The "cache read miss" is now the compiler step. Make it more explicit #1527

Merged
merged 3 commits into from Jan 6, 2023

Conversation

sylvestre
Copy link
Collaborator

@sylvestre sylvestre commented Jan 6, 2023

We are getting:

Average cache write               0.115 s
Average compiler write            5.737 s
Average cache read hit            0.000 s

was:

Average cache write               0.115 s
Average cache read miss           5.737 s
Average cache read hit            0.000 s

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2023

Codecov Report

Base: 30.58% // Head: 30.41% // Decreases project coverage by -0.16% ⚠️

Coverage data is based on head (da125cd) compared to base (a28f6c4).
Patch coverage: 22.22% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1527      +/-   ##
==========================================
- Coverage   30.58%   30.41%   -0.17%     
==========================================
  Files          48       48              
  Lines       16704    16704              
  Branches     7935     7931       -4     
==========================================
- Hits         5109     5081      -28     
- Misses       6206     6208       +2     
- Partials     5389     5415      +26     
Impacted Files Coverage Δ
src/compiler/compiler.rs 35.95% <20.00%> (+0.21%) ⬆️
src/server.rs 31.37% <25.00%> (-0.73%) ⬇️
src/test/utils.rs 36.36% <0.00%> (-4.05%) ⬇️
src/cache/disk.rs 35.61% <0.00%> (-1.37%) ⬇️
src/util.rs 34.74% <0.00%> (-1.30%) ⬇️
src/test/tests.rs 32.65% <0.00%> (-0.69%) ⬇️
src/compiler/c.rs 38.26% <0.00%> (-0.46%) ⬇️
src/compiler/gcc.rs 55.26% <0.00%> (-0.42%) ⬇️
src/cache/cache.rs 38.99% <0.00%> (-0.39%) ⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sylvestre sylvestre requested a review from drahnr January 6, 2023 11:52
Copy link
Collaborator

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Others LGTM

@@ -784,7 +784,7 @@ pub enum CompileResult {
CacheMiss(
MissType,
DistType,
Duration,
Duration, // Compilation time
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use BoxFuture?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry but why ? :)

Copy link
Collaborator

@Xuanwo Xuanwo Jan 6, 2023

Choose a reason for hiding this comment

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

Oh, no strong reasons, just to avoid repeating Pin Box things 🤣

@drahnr
Copy link
Collaborator

drahnr commented Jan 6, 2023

I think we should split the item into read miss, compile and cache write

@sylvestre
Copy link
Collaborator Author

I think we should split the item into read miss, compile and cache write

what do you mean? it is what we have currently

@drahnr
Copy link
Collaborator

drahnr commented Jan 6, 2023

I misinterpreted the changeset, if it's only compilation I'd name it as such compile, without the write suffix.
Question here is what does the compilation metric really give us?

@sylvestre
Copy link
Collaborator Author

I misinterpreted the changeset, if it's only compilation I'd name it as such compile, without the write suffix.

Good point, fixed

Question here is what does the compilation metric really give us?

It is free and interesting to see how long a user can save. And it is better to have it than not.

@sylvestre sylvestre merged commit 92a5678 into mozilla:main Jan 6, 2023
@sylvestre sylvestre deleted the rename branch January 6, 2023 21:41
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