It's not very easy to reason about overflow issues with our current representation. This means that we currently just pretend that they don't happen. The idea for changing the representation came up in response to a security bug where we didn't really have a better solution.
Changing to x1, x2, y1, y2 will allow us to match the pixman rectangle representation that we use for regions. The region code shows quite a bit profiles so even a small improvement in this area is nice to have. -Jeff On Wed, Feb 8, 2017 at 9:19 PM, David Major <dma...@mozilla.com> wrote: > Is there a specific problem that's being solved by this proposal? It would > be helpful to make this a bit more concrete, like "these benchmarks go x% > faster", or "here's a list of overflow bugs that will just vanish", or > "here's some upcoming work that this would facilitate". > > On Thu, Feb 9, 2017 at 1:56 PM, Botond Ballo <bba...@mozilla.com> wrote: >> >> Hi everyone! >> >> I would like to propose changing the internal representation of >> rectangles in platform code. >> >> We currently represent a rectangle by storing the coordinates of its >> top-left corner, its width, and its height. I'll refer to this >> representation as "x/y/w/h". >> >> I would like to propose storing instead the coordinates of the >> top-left corner, and the coordinates of the bottom-right corner. I'll >> refer to this representation as "x1/y1/x2/y2". >> >> The x1/y1/x2/y2 representation has several advantages over x/y/w/h: >> >> - Several operations are more efficient with x1/y1/x2/y2, including >> intersection, >> union, and point-in-rect. >> - The representation is more symmetric, since it stores two quantities >> of the >> same kind (two points) rather than a point and a dimension >> (width/height). >> - The representation is less susceptible to overflow. With x/y/w/h, >> computation >> of x2/y2 can overflow for a large range of values of x/y and w/h. >> However, >> with x1/y1/x2/y2, computation of w/h cannot overflow if the >> coordinates are >> signed and the resulting w/h is unsigned. >> >> A known disadvantage of x1/y1/x2/y2 is that translating the rectangle >> requires translating both points, whereas translating x/y/w/h only >> requires translating one point. I think this disadvantage is minor in >> comparison to the above advantages. >> >> The proposed change would affect the class mozilla::gfx::BaseRect, and >> classes that derive from it (such as CSSRect, LayoutRect, etc., and, >> notably, nsRect and nsIntRect), but NOT other rectangle classes like >> DOMRect. >> >> I would like to propose making the transition as follows: >> >> - Replace direct accesses to the 'width' and 'height' fields throughout >> the codebase with calls to getter and setter methods. (There aren't >> that many - on the order of a few dozen, last I checked.) >> >> - Make the representation change, which is non-breaking now that >> the direct accesses to 'width' and 'height' have been removed. >> >> - Examine remaining calls to the getters and setters for width and >> height and see if any can be better expressed using operations >> on the points instead. >> >> The Graphics team, which owns this code, is supportive of this change. >> However, since this is a fundamental utility type that's used by a >> variety of platform code, I would like to ask the wider platform >> development community for feedback before proceeding. Please let me >> know if you have any! >> >> Thanks, >> Botond >> >> [1] >> http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/gfx/2d/BaseRect.h#46 >> _______________________________________________ >> dev-platform mailing list >> dev-platform@lists.mozilla.org >> https://lists.mozilla.org/listinfo/dev-platform > > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
