Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upstylo: Avoid using InterpolateMatrix as a fallback for matched transform function pair #18966
+44
−9
Conversation
…orm function pair. In the current implementation, if there is any interpolation error in a matched transform function pair, we fall-back to use InterpolateMatrix unconditionally. However, the error could be caused by: 1. mismatched transform function pair 2. matched transform function pair within at least one undecomposable matrix. Using InterpolateMatrix for case 1 makes sense, however, using InterpolateMatrix for case 2 does not. According to the spec, we should just report error for case 2, and let the caller do the fallback procedure. Using InterpolateMatrix for case 2 will go through more unnecessary code path, and produce more memory usage and calculation cost, which should be avoidable. In this patch, we add an extra pass to check if a transform function pair have matched operations in advance. With this information, we can easily tell whether the interpolation error in a equal-length transform function pair is caused by case 1 or case 2. So, we can avoid the unnecessary cost. Gecko bug: Bug 1399049
highfive
commented
Oct 20, 2017
|
Heads up! This PR modifies the following files:
|
highfive
commented
Oct 20, 2017
|
|
|
This patch has been reviewed by @hiikezoe in Bug 1399049 already. |
|
|
|
@bors-servo r=hiikezoe |
|
|
|
|
wow. awesome @chenpighead |
bors-servo
added a commit
that referenced
this pull request
Oct 21, 2017
…toring, r=hiikezoe stylo: Avoid using InterpolateMatrix as a fallback for matched transform function pair In the current implementation, if there is any interpolation error in a matched transform function pair, we fall-back to use InterpolateMatrix unconditionally. However, the error could be caused by: 1. mismatched transform function pair 2. matched transform function pair within at least one undecomposable matrix. Using InterpolateMatrix for case 1 makes sense, however, using InterpolateMatrix for case 2 does not. According to the spec, we should just report error for case 2, and let the caller do the fallback procedure. Using InterpolateMatrix for case 2 will go through more unnecessary code path, and produce more memory usage and calculation cost, which should be avoidable. In this patch, we add an extra pass to check if a transform function pair have matched operations in advance. With this information, we can easily tell whether the interpolation error in a equal-length transform function pair is caused by case 1 or case 2. So, we can avoid the unnecessary cost. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Bug 1399049](https://bugzilla.mozilla.org/show_bug.cgi?id=1399049) - [X] These changes do not require tests because the change is for some performance gain, and we have tests to ensure that we won't regress the existing behavior already. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18966) <!-- Reviewable:end -->
|
|
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.
chenpighead commentedOct 20, 2017
•
edited by SimonSapin
In the current implementation, if there is any interpolation error in a matched
transform function pair, we fall-back to use InterpolateMatrix unconditionally.
However, the error could be caused by:
Using InterpolateMatrix for case 1 makes sense, however, using InterpolateMatrix
for case 2 does not. According to the spec, we should just report error for
case 2, and let the caller do the fallback procedure. Using InterpolateMatrix
for case 2 will go through more unnecessary code path, and produce more memory
usage and calculation cost, which should be avoidable.
In this patch, we add an extra pass to check if a transform function pair have
matched operations in advance. With this information, we can easily tell whether
the interpolation error in a equal-length transform function pair is caused by
case 1 or case 2. So, we can avoid the unnecessary cost.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is