Closed
Bug 1387514
Opened 7 years ago
Closed 7 years ago
Mechanical changes to BaseRect callers for width/height to Width()/Height()/SetWidth()/SetHeight() conversion
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: milan, Assigned: milan)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
(deleted),
text/x-review-board-request
|
botond
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
milan
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
aosmond
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
dholbert
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
overholt
:
review+
|
Details |
See bug 1386277 for motivation. This bug collects the easy to review/mechanical changes.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
Nit: could you adjust the commit message to describe the change? It's not clear what e.g. the sentence "dom/ files with rect width/height changes to accessors/setters" means.
I think you might to say something like:
"Upgrade BaseRect width & height member-var usages to instead use accessors & setters, in $DIRECTORY"
Flags: needinfo?(milan)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8895086 [details]
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in layout/*.
https://reviewboard.mozilla.org/r/166208/#review171468
r=me on this layout/ change, with the commit message adjusted to be actively-phrased & more readable/self-explanatory, per Comment 6.
Attachment #8895086 -
Flags: review?(dholbert) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #7)
> ...
>
> r=me on this layout/ change, with the commit message adjusted to be
> actively-phrased & more readable/self-explanatory, per Comment 6.
I like that, and change the rest of the commit messages to match.
Flags: needinfo?(milan)
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8895087 [details]
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in dom/*.
https://reviewboard.mozilla.org/r/166210/#review171826
I don't know the callsites here but things lgtm.
Attachment #8895087 -
Flags: review?(overholt) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8895084 [details]
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in .cpp files in gfx/*.
https://reviewboard.mozilla.org/r/166204/#review171862
I think some loops in FilterNodeSoftware need to be manually touched up to prevent a performance regression.
Other than that it looks okay to me. There could be other potential performance issues I missed, but I didn't see anything obvious.
Minusing for now because of the FilterNodeSoftware problem.
::: gfx/2d/FilterNodeSoftware.cpp:221
(Diff revision 2)
> int bpp = BytesPerPixel(aSurface->GetFormat());
>
> // Fill the first row by hand.
> if (bpp == 4) {
> uint32_t sourcePixel = *(uint32_t*)sourcePixelData;
> - for (int32_t x = 0; x < aFillRect.width; x++) {
> + for (int32_t x = 0; x < aFillRect.Width(); x++) {
This looks like a hot loop. Width() shouldn't be recomputed each iteration (not sure if Rect's are (x1,x2) yet but that'll make this worse). I'd hope compilers are smart enough to optimize this, but I'm not sure.
I think this should be manually touched up to compute Width() outside the loop.
I tried to mark all instances of this in the file but there are too many.
::: gfx/layers/composite/ContainerLayerComposite.cpp:44
(Diff revision 2)
>
> #define CULLING_LOG(...)
> // #define CULLING_LOG(...) printf_stderr("CULLING: " __VA_ARGS__)
>
> #define DUMP(...) do { if (gfxEnv::DumpDebug()) { printf_stderr(__VA_ARGS__); } } while(0)
> -#define XYWH(k) (k).x, (k).y, (k).width, (k).height
> +#define XYWH(k) (k).x, (k).y, (k).Width(), (k).Height()
I don't think that these macros are actually used. So it's odd this was added by the automated change.
Attachment #8895084 -
Flags: review?(rhunt) → review-
Comment 16•7 years ago
|
||
(In reply to Ryan Hunt [:rhunt] from comment #15)
> ::: gfx/layers/composite/ContainerLayerComposite.cpp:44
> (Diff revision 2)
> >
> > #define CULLING_LOG(...)
> > // #define CULLING_LOG(...) printf_stderr("CULLING: " __VA_ARGS__)
> >
> > #define DUMP(...) do { if (gfxEnv::DumpDebug()) { printf_stderr(__VA_ARGS__); } } while(0)
> > -#define XYWH(k) (k).x, (k).y, (k).width, (k).height
> > +#define XYWH(k) (k).x, (k).y, (k).Width(), (k).Height()
>
> I don't think that these macros are actually used. So it's odd this was
> added by the automated change.
I should add that this isn't an issue, I was just confused. The macros can probably be dropped, but they don't need to in this bug.
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Ryan Hunt [:rhunt] from comment #15)
...
>
> I think some loops in FilterNodeSoftware need to be manually touched up to
> prevent a performance regression.
> ...
> > - for (int32_t x = 0; x < aFillRect.width; x++) {
> > + for (int32_t x = 0; x < aFillRect.Width(); x++) {
>
> This looks like a hot loop. Width() shouldn't be recomputed each iteration
I would certainly expect the full optimization here given that Width() is "always inline", but I haven't looked at the created assembly.
You're right in that this should be changed when Width() becomes a computed value, but I wasn't worried about that in the initial pass.
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8895083 [details]
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in .h files in gfx/*.
https://reviewboard.mozilla.org/r/166202/#review171886
Looks good.
My only comment is, presumably once we switch to the x1/y1/x2/y2 representation, rect types will gain constructors that take x1/y1/x2/y2 arguments. In places where we construct a rect from x/y/w/h arguments taken from another rect, we'll probably want to use the x1/y1/x2/y2 constructor instead after the switch.
This patch touches call sites that fall into the above category, in the following files:
gfx/2d/Rect.h
gfx/src/nsRect.h
gfx/src/nsRegion.h
gfx/thebes/gfx2DGlue.h
I don't know whether we want to add a TODO comment about these, or just do another pass over all constructor calls after switching to x1/y1/x2/y2.
Attachment #8895083 -
Flags: review?(botond) → review+
Comment 19•7 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #17)
> (In reply to Ryan Hunt [:rhunt] from comment #15)
> ...
> >
> > I think some loops in FilterNodeSoftware need to be manually touched up to
> > prevent a performance regression.
> > ...
> > > - for (int32_t x = 0; x < aFillRect.width; x++) {
> > > + for (int32_t x = 0; x < aFillRect.Width(); x++) {
> >
> > This looks like a hot loop. Width() shouldn't be recomputed each iteration
>
> I would certainly expect the full optimization here given that Width() is
> "always inline", but I haven't looked at the created assembly.
>
> You're right in that this should be changed when Width() becomes a computed
> value, but I wasn't worried about that in the initial pass.
Ah I missed that they're marked MOZ_ALWAYS_INLINE and that we haven't switched to x1,y1,x2,y2 yet.
This is fine then.
I'd also be curious if they'd be optimized away even once the switch to x1,y1,x2,y2 happens. I just think it's less likely to always happen. I don't really know :)
Updated•7 years ago
|
Attachment #8895084 -
Flags: review- → review+
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Ryan Hunt [:rhunt] from comment #19)
> ...
>
> I'd also be curious if they'd be optimized away even once the switch to
> x1,y1,x2,y2 happens. I just think it's less likely to always happen. I don't
> really know :)
I think that should definitely be optimized at that time. These are just he "easy" patches to start. The rest will have to come slowly and be examined with more care, individually. May have to "hide" the direct access to .x/.y as well, just to make it clear if we're looking for translation (today) or just changing the left/bottom. Long haul :)
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8895085 [details]
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in image/*.
https://reviewboard.mozilla.org/r/166206/#review173408
Attachment #8895085 -
Flags: review?(aosmond) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8895084 [details]
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in .cpp files in gfx/*.
https://reviewboard.mozilla.org/r/166204/#review173426
Somehow r+ went back to r- with the rebase.
Attachment #8895084 -
Flags: review+
Assignee | ||
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8895084 [details]
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in .cpp files in gfx/*.
https://reviewboard.mozilla.org/r/166204/#review173430
Assignee | ||
Updated•7 years ago
|
Attachment #8895084 -
Flags: review?(rhunt)
Comment 29•7 years ago
|
||
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df7cada96cec
Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in .h files in gfx/*. r=botond
https://hg.mozilla.org/integration/autoland/rev/2a8f664f107e
Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in .cpp files in gfx/*. r=milan
https://hg.mozilla.org/integration/autoland/rev/d093907b21ad
Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in image/*. r=aosmond
https://hg.mozilla.org/integration/autoland/rev/1a5fb194a79c
Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in layout/*. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/39b6c51cebcd
Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in dom/*. r=overholt
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df7cada96cec
https://hg.mozilla.org/mozilla-central/rev/2a8f664f107e
https://hg.mozilla.org/mozilla-central/rev/d093907b21ad
https://hg.mozilla.org/mozilla-central/rev/1a5fb194a79c
https://hg.mozilla.org/mozilla-central/rev/39b6c51cebcd
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Assignee: nobody → milaninbugzilla
You need to log in
before you can comment on or make changes to this bug.
Description
•