Issue 6923048: Minor optimizations to SkMatrix44. (Closed)
|
Created:
11 years, 11 months ago by shawnsingh Modified:
11 years, 11 months ago CC:
skia-review_googlegroups.com, whunt, vollick, jamesr Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionOptimized implementation of SkMatrix44::preScale
preScale() does not need to do a general matrix multiply. It can do 4x less
arithmetic by directly scaling the columns of the input matrix.
Committed: https://code.google.com/p/skia/source/detail?r=6807
Patch Set 1 #
Total comments: 10
Patch Set 2 : prescale implementation only #Patch Set 3 : #MessagesTotal messages: 13
Tom had a good suggestion to add a validate() method for debug mode, since we're trying to be more aggressive about using the bit flags assuming they remain correct. new patch ETA tomorrow
Sign in to reply to this message.
https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp File src/utils/SkMatrix44.cpp (right): https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:410: this->setTypeMask(a_mask | b_mask); We need to be careful with this, since on SkMatrix we have some flags that can't be or'ed. https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:557: inverse->setTypeMask(this->getType()); Likewise here - if we ever get a rect-stays-rect equivalent, for example. Not yet a problem, however.
Sign in to reply to this message.
Lets reference or add new benchs to show the speedups. https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp File src/utils/SkMatrix44.cpp (right): https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:238: slightly risky that [3][0..2] may all be zero. Should we confirm that before setting the translate bit? https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:256: fMat[3][2] += dz; Its possible that our [3][0..2] are all zero now, so we can't blindly mark it as translate. https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:297: Not sure we'll always be in a scale afterwards. Lets try writing as a loop, as we do in postScale. If its not measurably slower, its easier to read. https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:310: } ditto about not being sure we're scaled afterwards https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:410: this->setTypeMask(a_mask | b_mask); On 2012/12/11 09:30:15, TomH wrote: > We need to be careful with this, since on SkMatrix we have some flags that can't > be or'ed. And we may not be as complex as the union of our two inputs. I don't think this is safe. https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:557: inverse->setTypeMask(this->getType()); On 2012/12/11 09:30:15, TomH wrote: > Likewise here - if we ever get a rect-stays-rect equivalent, for example. Not > yet a problem, however. We had numerical instabilities before where this assertion (the inverse type == orig type) didn't always hold.
Sign in to reply to this message.
Thanks for review. =) https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp File src/utils/SkMatrix44.cpp (right): https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:238: On 2012/12/11 14:16:53, reed1 wrote: > slightly risky that [3][0..2] may all be zero. Should we confirm that before > setting the translate bit? It seems like I'm making a bad assumption about the bit flags - that it's OK to have the flag indicate an operation exists, even if the operation is a no-op (i.e. translate by zero, scale by 1, etc) If that's not the semantics you had intended, then I think we should abandon these proposed bitflag changes =) https://codereview.appspot.com/6923048/diff/1/src/utils/SkMatrix44.cpp#newcod... src/utils/SkMatrix44.cpp:297: On 2012/12/11 14:16:53, reed1 wrote: > Not sure we'll always be in a scale afterwards. > > Lets try writing as a loop, as we do in postScale. If its not measurably slower, > its easier to read. OK, will try the loop.
Sign in to reply to this message.
a benchmark for prescale implementation already exists. results on my fast desktop machine: current implementation: 5.17 for-loop implementation: 0.70 unrolled implementation: 0.70 next patch I'm submitting only the for-loop version, without any bit-flag changes.
Sign in to reply to this message.
PTAL, thanks! jamesr@ If we can land this tonight do you think it will roll in before branch?
Sign in to reply to this message.
LGTM, I'm in the UK and can test & commit in 10 hours if NC or MTV don't get a chance. We've been told to expect that anything landing in the next 24 hours is guaranteed to make branch, right?
Sign in to reply to this message.
LGTM, I'm in the UK and can test & commit in 10 hours if NC or MTV don't get a chance. We've been told to expect that anything landing in the next 24 hours is guaranteed to make branch, right?
Sign in to reply to this message.
On 2012/12/13 22:12:45, TomH wrote: > LGTM, I'm in the UK and can test & commit in 10 hours if NC or MTV don't get a > chance. We've been told to expect that anything landing in the next 24 hours is > guaranteed to make branch, right? Thanks for review, I'll try to land it now.
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
On Thu, Dec 13, 2012 at 2:12 PM, <tomhudson@google.com> wrote: > LGTM, I'm in the UK and can test & commit in 10 hours if NC or MTV don't > get a chance. We've been told to expect that anything landing in the > next 24 hours is guaranteed to make branch, right? > If it rolls into chromium in that timeframe then it should definitely make the m25 branch (Monday night PST). > > https://codereview.appspot.**com/6923048/<https://codereview.appspot.com/6923... >
Sign in to reply to this message.
We land to do a roll tomorrow morning. On Thu, Dec 13, 2012 at 5:21 PM, James Robinson <jamesr@google.com> wrote: > > > On Thu, Dec 13, 2012 at 2:12 PM, <tomhudson@google.com> wrote: >> >> LGTM, I'm in the UK and can test & commit in 10 hours if NC or MTV don't >> get a chance. We've been told to expect that anything landing in the >> next 24 hours is guaranteed to make branch, right? > > > If it rolls into chromium in that timeframe then it should definitely make > the m25 branch (Monday night PST). > >> >> >> https://codereview.appspot.com/6923048/ > >
Sign in to reply to this message.
|
