Skip to content

Optimize diagrams. #851

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

Closed
wants to merge 2 commits into from
Closed

Optimize diagrams. #851

wants to merge 2 commits into from

Conversation

davidlehn
Copy link
Contributor

This uses SVGO to optimize the diagrams/ SVG files. The provided svgo.config.js disables the desc and title plugins so that text is preserved. It also keeps newlines and uses a simple 1 space indent for readability. Other SVGO options and plugins might improve the output, I didn't explore them.

$ du -b diagrams diagrams-orig
625668  diagrams
1062712 diagrams-orig

See discussion at:
#721

If the original diagrams are the preferred format for editing then this change should not be done and it should instead be a build step at some point before publishing. Also worth considering if network compression and target display devices make this optimization unneeded.

@davidlehn davidlehn mentioned this pull request Dec 9, 2021
@chaals
Copy link
Contributor

chaals commented Dec 15, 2021

I would prefer we don't do this. If you look at the gains for an SVG that I have already optiimsed such as presentation-graph.svg they are minimal - it removes some whitespace, and it expands " to "

(and it uses the <![CDATA[ ...styles... ]]> bit from XML properly, which I suppose is useful if someone is using a proper XML-compliant tool to read SVG instead of a browser).

All that is a) trivial and b) can be done better by hand.

The gains are all in the files that are generated by Google - which are awful, but using SVGO makes them even harder to clean up.

@chaals
Copy link
Contributor

chaals commented Dec 15, 2021

Diagrams directory, with the un-optimsed versions:

diagrams $ ls -l
total 2136
-rw-r--r--     1131 Aug 30 08:00 claim-example-2.svg
-rw-r--r--     1131 Aug 30 08:05 claim-example.svg
-rw-r--r--     1801 Aug 30 09:38 claim-extended.svg
-rw-r--r--     1013 Aug 30 08:10 claim.svg
-rw-r--r--     4676 Aug 30 07:25 credential-graph.svg
-rw-r--r--    34750 Aug 30 07:25 credential.svg
-rw-r--r--   178398 Aug 30 07:25 ecosystem.svg
-rw-r--r--   136841 Aug 30 07:25 ecosystemdetail.svg
-rw-r--r--     7471 Aug 30 07:25 presentation-graph.svg
-rw-r--r--    47363 Aug 30 07:25 presentation.svg
-rw-r--r--   103538 Aug 30 07:25 privacy-spectrum.svg
-rw-r--r--   230239 Aug 30 07:25 subject-ne-holder.svg
-rw-r--r--   312218 Aug 30 07:25 zkp-cred-pres.svg

The biggest hand-cleaned file is < 7,5k - and it's the most complex image in the spec. All the files over 10k (34k-300k) are the ones from Google. All the rest are the ones I tweaked.

@davidlehn
Copy link
Contributor Author

To be clear, I have no real attachment to this change, and it's fine to close. It was requested in #721, and was easy to do, so I hope it at least provided some data points.

@msporny The discussion in the other issue had me thinking this would be closed, but it's marked for merge? I don't even like this PR vs a build step method!

My main concern had been from a while ago when I noticed the spec was sending 1.7MB of svg for just a few simple images. That's better now at 1MB. Still large, but I guess ok? SVGO drops it a bit more, but with diminishing returns for what I expect is the target audience.

I would guess that SVG simple enough to optimize by hand is already small enough that it isn't a big deal size wise. (Though I do appreciate the beauty of clean hand written artisanal SVG!) The large files are another matter but are (or could be) too difficult to edit by hand, and also allow editing by graphics programs. I had assumed SVGO would be used in a build step, with editing happening on originals, rather than the overwrite commit presented here. Avoiding build step complexity would be nice.

@kdenhartog
Copy link
Member

I don't have a major opinion on this, but these are the two guiding principles I'm operating under right now:

  1. Smaller is better to save on size of data downloaded during reading
  2. Automated is ideal, but given how infrequently these images need to change I'm alright with manual updates

I tend to agree with the original opinion on issue that we go ahead and close this PR without merging.

@msporny
Copy link
Member

msporny commented Dec 16, 2021

I've removed the merge label, as I'm reading some of these comments as low key objections. I do want to make a point that I haven't made yet, though:

I find it strange that people feel like optimizing 700KB out of a spec is worth our time given that we regularly stream multiple hundreds of GBs watching shows online. Feels like we're over-engineering the optimization of images in a specification that a vanishingly small portion of the global population is going to read when we could be spending on our time on things that will benefit a greater percentage of humanity. :)

If we're working on this for a11y reasons, great -- that's worth our time. If we're working on this to save a few bytes -- that's of questionable value.

@chaals
Copy link
Contributor

chaals commented Dec 16, 2021

I find it strange that people feel like optimizing 700KB out of a spec is worth our time given that we regularly stream multiple hundreds of GBs watching shows online. [ rationale snipped ]

If we're working on this for a11y reasons, great -- that's worth our time. If we're working on this to save a few bytes -- that's of questionable value.

It's partly because for me the difference in size is meaningful. I don't stream GB of television, and I pay for data "by the kilo" (or ounce in the US where it's horrendous as a visitor). I realise I am a minority, but that's OK since I'm the minority taking on the hard work, which doesn't have any negative impact on the majority who don't care.

It's partly for accessibility - there is a substantial but not infinite improvement easily achievable by making the hand-coded versions. More to the point, it is much easier to work with, which makes further improvements to accessibility feasible.

@msporny
Copy link
Member

msporny commented Dec 27, 2021

Ok, so just to clarify, we're closing this PR and issue #721 with no changes. We will all have to remember that we have some hand crafted SVG associated with the spec, so if we ever go to re-do the diagrams (and re-export from Google Docs), we'll need to be cognizant of that. Given that I've been the one to do that in the past, I think I can remember (and if I don't, we have version control and can revert if I mess that up).

@msporny msporny added the DO NOT MERGE PR contains something that should not be merged. label Dec 27, 2021
@chaals
Copy link
Contributor

chaals commented Dec 28, 2021

Works for me. If you remember to tag me when you want to play with the diagrams, we can smooth the path... and if not, as you say, someone can remind us :)

@msporny msporny added the pending close Close if no objection within 7 days label Dec 28, 2021
@msporny
Copy link
Member

msporny commented Dec 28, 2021

Just marked this as "close after 7 days", and will close sooner if we get confirmation from @davidlehn that he's ok w/ closing this and issue #721.

@davidlehn
Copy link
Contributor Author

@msporny Yes, it's fine to close this and related issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE PR contains something that should not be merged. pending close Close if no objection within 7 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants