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)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: milan, Assigned: milan)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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/*.
59 bytes, text/x-review-board-request
botond
: review+
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/*.
59 bytes, text/x-review-board-request
milan
: review+
Details
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in image/*.
59 bytes, text/x-review-board-request
aosmond
: review+
Details
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in layout/*.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1387514: Upgrade BaseRect (derived classes) width and height direct member variable use to instead use Width()/SetWidth() and Height()/SetHeight() in dom/*.
59 bytes, text/x-review-board-request
overholt
: review+
Details 
See bug 1386277 for motivation.  This bug collects the easy to review/mechanical changes.
Blocks: 1386277
No longer depends on: 1386277
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 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+
(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 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 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-
(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.
(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 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+
(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 :)
(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 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 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+
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
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
Assignee: nobody → milaninbugzilla
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: