Skip to content

doc.rust-lang.org: set a long cache-control for fonts #108

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 1 commit into from

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Jun 23, 2022

Context: On all rustdoc pages there are 6+ custom fonts. These are on the critical rendering path; text can't start being displayed without them. On docs.rs, they are served with a very long Cache-Control max-age, so they are almost never requested by the browser. On doc.rust-lang.org, they are served with no Cache-Control header. They do have an ETag, so in practice most page navigations result in an HTTP 304 Not Modified and the fonts don't have to be redownloaded. But the time waiting for that request and response still delays rendering, and it would be better if the browser could immediately load the fonts from its local cache.

The CSS and the storage JS are also on the critical path, but their cache policy is a little less straightforward since they can change with each release. I figured I'd start with the fonts, which never change, and see if this is a good approach and works well.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jun 23, 2022

I'm a little worried that a blanket policy like this will make it hard for us to fix bugs (e.g., if we ship a bad version for whatever reason). Is there some hash or other version ID in the filename for these fonts? If not, can we make a change to rustdoc to add that? Even a hash of the file contents would work, I think.

@jsha
Copy link
Contributor Author

jsha commented Jun 23, 2022

There's not currently a version number or hash in the font filenames, but it would be easy to add one to future revisions of those files (which are quite rare). It does seems like a good idea to add a general hash ID to files that are meant for cacheability, though. I'll file an issue for rustdoc and discuss with the team.

Given that it's easy to make new versions of these files have new names, do you feel it's a blocker to have the rustdoc changes done before trying this out?

@jsha
Copy link
Contributor Author

jsha commented Jun 23, 2022

Filed an issue for rustdoc filenames: rust-lang/rust#98413

@Mark-Simulacrum
Copy link
Member

Can you say more about "easy to make new versions"? Do you mean just that we can pretty easily manually name them something different? (And there's no dependency on names being constant in docs.rs for example)?

If so, then this seems ok to me.

@jsha
Copy link
Contributor Author

jsha commented Jun 24, 2022

Can you say more about "easy to make new versions"? Do you mean just that we can pretty easily manually name them something different?

Yes, that's correct. And in fact these files are categorized in rustdoc as Unversioned: "This file will never change, no matter what toolchain is used to build it."

https://github.com/rust-lang/rust/blob/fdca237d5194bf8a1c9b437ebd2114d1c2ba6195/src/librustdoc/html/render/write_shared.rs#L23-L48

there's no dependency on names being constant in docs.rs for example?

Nope. docs.rs does load fonts by these filenames in its additional CSS, but that will continue to work if font filenames change. That would result in a little double-loading, but (a) because they are well cached it's not a big deal, and (b) that CSS can be updated in the (unlikely) event of a change, to reduce the double-loading.

@Mark-Simulacrum
Copy link
Member

Ok, that all sounds good. I think we should still fix this to version them (even manually - embedding a hash into the filename we always use) in rustdoc, but I'm comfortable deploying this in the meantime as a first step. I'll do so over the weekend most likely.

@jsha
Copy link
Contributor Author

jsha commented Jun 24, 2022

Excellent, thanks! One caveat: I haven't been able to test this live because I don't have access to a staging environment. I get as far as terraform init and get prompted for credentials.

If I hook this up to my AWS account and run terraform apply in order to test it, is it likely to succeed in creating a similar environment to prod such that I could verify the change?

@Mark-Simulacrum
Copy link
Member

Not really. I wouldn't worry about it too much, I'll probably start with a lower increase and do some loose checking.

Eventually we'll want proper staging but we haven't gotten there yet.

I don't think we have enough in terraform to make deployments from local environments possible - there's definitely a good bit that's still purely in the console.

Mark-Simulacrum added a commit that referenced this pull request Jun 25, 2022
This brings in a few new resources, which will be useful when merging #108.

The code delta is a no-op at plan time (after importing the new resources from
S3 bucket resource expansion in 4.x).
@Mark-Simulacrum
Copy link
Member

Hm, I guess GitHub didn't pick it up, but cherry-picked this onto master and slightly adjusted. We're now caching woff2:

  • For 24 hours in CloudFront edge locations (by the Managed-Cachingoptimized policy)
  • For 24 hours in browsers (by the custom response headers policy)
    • This is also set to permit re-using stale results for an additional 24 hours

I think this is enough that we can probably expect ~zero blocking requests, and if no problems are reported we can consider bumping the policy even higher.

@jsha
Copy link
Contributor Author

jsha commented Jun 26, 2022 via email

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.

2 participants