[Impeller] Performance improvement for Path::GetMinMaxCoveragePoints. #37827
Conversation
This method was visible in profiles.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| } | ||
|
|
||
| for (const auto& quad : quads_) { | ||
| clamp(quad.Extrema()); | ||
| for (const Point& point : quad.Extrema()) { |
There was a problem hiding this comment.
I didn't think this would be all that expensive. But finding the extrema for all components is overkill. Since the curve would lie entirely within the convex hull of its control points, perhaps we should just cycle over the control points instead? This would make coverage calculation way faster for not a lot of wastage.
cc @bdero
There was a problem hiding this comment.
I'm a bit worried that the degenerate case is a bit too extreme for min/maxing the control points (at least for cubics):
Green is coverage as it's currently computed.
I took a look over this code and noticed other easy improvements we can make: For cubics, we find coverage by taking the analytical derivative (a quadratic curve) and then finding the up-to-2 solutions for y=0 for each dimension -- we can improve things here by keeping more stuff on the stack, but so far so good.
But for quadratics, QuadraticPathComponent::Extrema() "upgrades" the curve to cubic form and then finds the cubic extrema. Instead, we should just take the derivative of the quadratic (a line) and include the y=0 solution for each direction.
There was a problem hiding this comment.
We should file an issue for that. In the meantime, this patch is good to go.
chinmaygarde
changed the title
jason-simmons
added
the
autosubmit
…116301) * 280e24b3e Remove use of stringstream in NormalizeUniformKey (flutter/engine#37826) * d8f151e24 Performance improvement for Path::GetMinMaxCoveragePoints (flutter/engine#37827)
…lutter#116301) * 280e24b3e Remove use of stringstream in NormalizeUniformKey (flutter/engine#37826) * d8f151e24 Performance improvement for Path::GetMinMaxCoveragePoints (flutter/engine#37827)
…lutter#116301) * 280e24b3e Remove use of stringstream in NormalizeUniformKey (flutter/engine#37826) * d8f151e24 Performance improvement for Path::GetMinMaxCoveragePoints (flutter/engine#37827)
…lutter#116301) * 280e24b3e Remove use of stringstream in NormalizeUniformKey (flutter/engine#37826) * d8f151e24 Performance improvement for Path::GetMinMaxCoveragePoints (flutter/engine#37827)
This method was visible in profiles.