-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Improve recognition of text MIME types for compression #360
Improve recognition of text MIME types for compression #360
Conversation
Review changes with SemanticDiff. Analyzed 1 of 3 files. Overall, the semantic diff is 34% smaller than the GitHub diff.
|
@@ -36,33 +38,19 @@ use crate::{ | |||
Result, | |||
}; | |||
|
|||
/// Contains a fixed list of common text-based MIME types in order to apply compression. | |||
pub const TEXT_MIME_TYPES: [&str; 24] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this was public, so this is a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the comment open for mentioning this breaking change in the next release.
Benchmark results show a clear improvement on small text files (going down in the noise on large ones). Seemingly no real change on the directory index ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -36,33 +38,19 @@ use crate::{ | |||
Result, | |||
}; | |||
|
|||
/// Contains a fixed list of common text-based MIME types in order to apply compression. | |||
pub const TEXT_MIME_TYPES: [&str; 24] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the comment open for mentioning this breaking change in the next release.
This caused a significant performance regression on directory listings for some reason. |
Ah, I see – not actually a regression. Previously, we wouldn’t compress directory listings because the original logic wouldn’t recognize |
It would be expected, but is there a big margin? |
Sure, compression takes time (gzip in particular which is what vegeta is testing). So directory listing tests went up by almost 50%. |
Description
If you look at the list of MIME types potentially produced by the
mime_guess
package, the list incompression::TEXT_MIME_TYPES
isn’t really great for recognizing text MIME types. This implements the following changes:text/
or ending in+xml
/+json
HashSet
charset
parameter (a bug I mentioned in Apply the usual post-processing to error responses for consistency #359)mime_guess
and removed the outdatedapplication/x-javascript
typeNote that
lazy_static
isn’t a new dependency, we already have it as a transitive dependency viatracing-subscriber
.How Has This Been Tested?
Functional testing on Linux looks fine, and Rust files (
text/x-rust
) are now being compressed as well.