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

Lint: Optimize images to reduce file size by 1.5-2x for free #2542

Merged
merged 2 commits into from
Apr 23, 2022

Conversation

CAM-Gerlach
Copy link
Member

Currently, many of the PNG images in PEPs (including some "hot" PEPs, like PEP 1) were not optimized, wasting bandwidth and increasing page load time. If we suppose PEP 1 gets 50 views per day, the ≈50 kB that could be saved on its 1 image alone costs nearly 1 GB of bandwidth per year (minus caching).

Compressing them more efficiently with optipng makes them between 1.5x and 2x smaller for free, with zero effect on quality, resolution or client-side support/performance, so this seems like an easy win.

@CAM-Gerlach CAM-Gerlach added the lint Linter-related work and linting fixes on PEPs label Apr 20, 2022
@CAM-Gerlach CAM-Gerlach self-assigned this Apr 20, 2022
@AA-Turner
Copy link
Member

AA-Turner commented Apr 20, 2022

For PEP 1, we could remove the raster image and use SVG now we're on Sphinx (https://github.com/python/peps/blob/main/pep-0001/process_flow.svg)?

(That SVG file isn't very optimised though, so perhaps a bad example!)

A

@gpshead
Copy link
Member

gpshead commented Apr 20, 2022

For PEP 1, we could remove the raster image and use SVG now we're on Sphinx (https://github.com/python/peps/blob/main/pep-0001/process_flow.svg)?

I wouldn't bother.

@CAM-Gerlach
Copy link
Member Author

And considering the SVG is 27 kB, whereas the optimized PNG is only 8 kB (even the unoptimized version is 12 kB), I'm not so sure about that, and SVG optimization is a lot less trivial than PNG optimization. Also, we'd have to test how it would interact with the color-inversion filter in dark mode, and I seem to remember running into another specific issue using a SVG in this context under the modern build system a while back that I can't recall the specifics of, so we'd need to do some testing first.

@AA-Turner
Copy link
Member

A quick go with limited XML/SVG knowledge (down to 2.4kB), but pragmatism may win the day xref being easier:

<svg xmlns="http://www.w3.org/2000/svg" width="690.67" height="306.67" viewBox="0 0 518 230">
<style>text{font:22px sans-serif}marker{overflow:visible}path{stroke:#000}</style>
<defs>
<rect id="Y" width="127" height="37" fill="none" stroke="#000" stroke-width="1.15"/>
<path id="Z" d="M0 0l5-5-17.5 5L5 5 0 0z" transform="scale(-.4) translate(10)" fill="#000" stroke-width="1"/>
<marker id="A" orient="auto"><use href="#Z"/></marker>
<marker id="B" orient="auto"><use href="#Z"/></marker>
<marker id="C" orient="auto"><use href="#Z"/></marker>
<marker id="D" orient="auto"><use href="#Z"/></marker>
<marker id="E" orient="auto"><use href="#Z"/></marker>
<marker id="F" orient="auto"><use href="#Z"/></marker>
<marker id="G" orient="auto"><use href="#Z"/></marker>
<marker id="H" orient="auto"><use href="#Z"/></marker>
<marker id="I" orient="auto"><use href="#Z"/></marker>
<marker id="J" orient="auto"><use href="#Z"/></marker>
<marker id="K" orient="auto"><use href="#Z"/></marker>
</defs>
<use href="#Y" x="99.5"  y="58.8" /><text x="103.6" y="85.5" >Provisional</text>
<use href="#Y" x="10.5"  y="10.6" /><text x="45.5"  y="37.3" >Draft</text>
<use href="#Y" x="221.6" y="155"  /><text x="226.5" y="181.8">Withdrawn</text>
<use href="#Y" x="160.8" y="106.9"/><text x="176.3" y="131.5">Rejected</text>
<use href="#Y" x="374"   y="10.6" /><text x="412"   y="37.3" >Final</text>
<use href="#Y" x="190.5" y="10.6" /><text x="203.7" y="35.2" >Accepted</text>
<use href="#Y" x="10.5"  y="185.7"/><text x="25.7"  y="212.4">Deferred</text>
<use href="#Y" x="374"   y="185.7"/><text x="404"   y="212.4">Active</text>
<use href="#Y" x="374.5" y="98.2" /><text x="387.4" y="122.8">Replaced</text>
<g fill="none" stroke-width="1.5">
<path marker-end="url(#K)" d="M17.5 48.3v134.8"/>
<path marker-end="url(#J)" d="M26.6 186V49.5"/>
<path marker-end="url(#I)" d="M439.2 47.4v49.3" stroke-dasharray="3,1.5"/>
<path marker-end="url(#H)" d="M137.1 29.7h52"/>
<path marker-end="url(#G)" d="M318.4 27.5h52"/>
<path marker-end="url(#F)" d="M69.8 48.5v29.2h27.2"/>
<path marker-end="url(#E)" d="M56 47.4v76.4h102.3"/>
<path marker-end="url(#D)" d="M42.1 47.3v126.5h178"/>
<path marker-end="url(#C)" d="M226.4 69.1h176.7V50.2"/>
<path marker-end="url(#B)" d="M226.9 85.5h29.8v20.1" stroke-dasharray="3,1.5"/>
<path marker-end="url(#A)" d="M226.4 77.8h90.2v75.6" stroke-dasharray="3,1.5"/>
</g>
</svg>

A

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Not essential, we can wring a few extra bytes with these options:

for f in $(find . -name '*.png'); do optipng -o7 -zm1-9 $f; done

@hugovk
Copy link
Member

hugovk commented Apr 20, 2022

PNGs

I use https://imgbot.net/ in other projects which opens PRs to auto-compresses images as needed. For example:

Something to consider here?

SVG

I would switch to SVG, the diagram was originally added as an SVG in #577 but replaced with a PNG in #701 / #702 because problems with the old rendering system. But the new rendering system is good with SVGs :)

SVG is more accessible, https://jakearchibald.github.io/svgomg/ can get it down from 27k to 15k.

The SVG is also easier to edit for future changes, so we can delete the exported PNG.

And the paragraph after the diagram explains there are missing states:

While not shown in the diagram, “Accepted” PEPs may technically move to “Rejected” or “Withdrawn” even after acceptance. This will only occur if the implementation process reveals fundamental flaws in the design that were not noticed prior to acceptance of the PEP. Unlike Provisional PEPs, these transitions are only permitted if the accepted proposal has not been included in a Python release - released changes must instead go through the regular deprecation process (which may require a new PEP providing the rationale for the deprecation).

I think the accessibility and editing advantages outweigh a difference of a few kilobytes here or there, especially when the whole page is 57 kB (and PEP 0 is 308 kB). And we're not talking about super-high traffic page like the frontpage of https://docs.python.org/.

Mermaid

Just sharing this as an experiment, not a suggestion for use here :)

GitHub now supports Mermaid for diagrams in Markdown. To play with it I re-made the PEP 1 flowchart.

Here's the raw thing:

  flowchart LR
      Draft --> Accepted
      Draft --> Provisional
      Draft --> Rejected
      Draft --> Withdrawn
      Draft <--> Deferred
      Accepted --> Final
      Final -.-> Replaced
      Provisional --> Final
      Provisional -.-> Rejected
      Provisional -.-> Withdrawn
      Active

And adding mermaid to the backticks:

  flowchart LR
      Draft --> Accepted
      Draft --> Provisional
      Draft --> Rejected
      Draft --> Withdrawn
      Draft <--> Deferred
      Accepted --> Final
      Final -.-> Replaced
      Provisional --> Final
      Provisional -.-> Rejected
      Provisional -.-> Withdrawn
      Active
Loading

Adding the missing states:

  flowchart LR
      Draft --> Accepted
      Draft --> Provisional
      Draft --> Rejected
      Draft --> Withdrawn
      Draft <--> Deferred
      Accepted --> Final
      Accepted --> Rejected
      Accepted --> Withdrawn
      Final -.-> Replaced
      Provisional --> Final
      Provisional -.-> Rejected
      Provisional -.-> Withdrawn
      Active
  flowchart LR
      Draft --> Accepted
      Draft --> Provisional
      Draft --> Rejected
      Draft --> Withdrawn
      Draft <--> Deferred
      Accepted --> Final
      Accepted --> Rejected
      Accepted --> Withdrawn
      Final -.-> Replaced
      Provisional --> Final
      Provisional -.-> Rejected
      Provisional -.-> Withdrawn
      Active
Loading

@AA-Turner
Copy link
Member

AA-Turner commented Apr 21, 2022

Hugo raises a good point -- we could actually replace the image with a representation as a graph. Sphinx has native support for Graphviz's .dot graphs, which is a widely-used format.

A

@gpshead
Copy link
Member

gpshead commented Apr 21, 2022

Very cool. though as a general point I suggest going ahead and merge this PR focused solely on the png optimizations. Make a follow on PR to change relevant things to a nice new graph format rather than piling on different types of changes into what will wind up as a single commit.

@CAM-Gerlach
Copy link
Member Author

I just dropped the optimized PNGs for those images (currently just the PEP 1 graph plus pep-0495-fold-2.png and pep-0495-gap.png) and swapped them over to use the existing (unoptimized) SVGs, after testing them to ensure they displayed as intended (and indeed, the existing one in PEP 532 renders just fine). A followup PR can explore optimizing them, changing them to use graph objects, etc.

To note, if we're going to redo it, we should fix the diagram itself; it contains a non-existent "Replaced" status (that should presumably be "Superseded") and ensuring Draft --> -.-> Accepted -.-> Active --> Final.

I use https://imgbot.net/ in other projects which opens PRs to auto-compresses images as needed. For example:

Something to consider here?

Thus far images are fairly infrequently used on PEPs (only a few dozen total over all ≈700 PEPs), and only optimizing after they are committed to the repo proper would permanently inflate the repo size. if we're going to do something like that, we should optimize them automatically before they hit main, either via pre-commit locally or (similar to e.g. what we have on docrepr) we do on, e.g. docrepr), via a GH action committing to the PR branch on request.

optipng -o7 -zm1-9

I typically use that for particularly "hot" images, or those small enough that the extra runtime doesn't matter; in this case, considering most of these older PEPs aren't accessed that frequently, a handful of bytes was less important than not permanently inflating the repo size and waiting several minutes per image for my old laptop to churn through each one.

though as a general point I suggest going ahead and merge this PR focused solely on the png optimizations. Make a follow on PR to change relevant things to a nice new graph format rather than piling on different types of changes into what will wind up as a single commit.

Agreed with respect to more substantial content changes, though I'd rather not pay the cost of having the optimized versions stuck in the repo forever if they're going to be replaced right away, so I switched those to the existing SVGs and just removed the PNGs instead.

@CAM-Gerlach CAM-Gerlach force-pushed the lint-optimize-images branch from cc5f2b7 to 489d69e Compare April 21, 2022 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint Linter-related work and linting fixes on PEPs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants