Normalize each import before it is cached. #392
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is one step in a series of commits to update the Haskell implementation to match the import semantics standardized in: dhall-lang/dhall-lang#127 This is a BREAKING CHANGE mostly due to restricting the grammar for valid file paths and URLs. However, in practice most existing code should still work after these changes. Specifically, this change: * Updates the grammar for file paths and URLs * Simplifies and cleans up the semantics for canonicalizing paths (and chains of paths) The main difference in the grammar is that file paths and URL paths are unified to have the same syntax and they are both more restricted than before. For example, the following URLs are no longer valid: * `http://example.com` (missing a path to a file) * `http://example.com/()` (invalid path characters) ... and the following paths are no longer valid: * `/?#` (invalid path characters) Note that this actually deviates from the standard in one way: file paths no longer permit `?` and `#` characters (whereas the standard allows them). However, in the course of testing these changes I realized that if file paths permit `?` and `#` characters and you unify URLs to also permit them then you get an ambiguous grammar where a `?` or `#` in a URL could be interpreted as either part of a path component or part of a query parameter or fragment. The semantics for canonicalizing paths is a straightforward translation of the standard to Haskell code.
In tests on Dhall configs we use at work, I saw speedups between 33–90% on extremely complicated configs with this change. It mostly just adds a line calling `normalize`, but also threads the Normalizer through a bunch of places to make sure we normalize properly for the context. This is based on dhall-lang#390, because this optimization is likely less effective if we’re doing more imports.
This was referenced May 17, 2018
|
Thanks! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In tests on Dhall configs we use at work, I saw speedups between 33–90% on
extremely complicated configs with this change.
It mostly just adds a line calling
normalize, but also threads the Normalizerthrough a bunch of places to make sure we normalize properly for the context.
This is based on #390, because this optimization is likely less effective if
we’re doing more imports.