Skip to content

Conversation

@MeGaGiGaGon
Copy link
Collaborator

Description

Trying the change from #4771 , I'd like to see if this helps reduce the diff-shades runtime, since the 22% speed increase from #4771 could be enough to make the action consistent again.

Checklist - did you ...

  • Implement any code style changes under the --preview style, following the
    stability policy?
  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@MeGaGiGaGon MeGaGiGaGon added the skip news Pull requests that don't need a changelog entry. label Oct 6, 2025
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Seems fine to do, could also add a changelog entry since this likely will help perf in some cases.

I think Shantanu commented that we could reuse stat results from the cache logic, that might speed it up a bit more (avoiding syscalls).

@MeGaGiGaGon MeGaGiGaGon removed the skip news Pull requests that don't need a changelog entry. label Oct 14, 2025
@ewest2350-ux

This comment has been minimized.

@github-actions
Copy link

diff-shades reports zero changes comparing this PR (2c67c00) to main (b895a73).


What is this? | Workflow run | diff-shades documentation

@cobaltt7
Copy link
Collaborator

cobaltt7 commented Oct 26, 2025

Looking at diff-shades, there is a noticeable difference in speed when running bigger files first. Stable style went from 6:51 to 4:23. However, preview style only went from 7:19 to 7:10.
Also, it may not be worth it to try reusing the stats from the cache logic; they have to be passed between functions in weird ways. I got it working currently, but the cache tests need to be changed.
I'm not sure how reliable the old diff-shades run from before stats were reused is, but in that run, stable style increased from 6:13 to 6:23.

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.

4 participants