Current Pixman region code shows up in profiles a lot

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We could replace it with our own region code, which may be easier to optimize and removes another dependency on externally maintained and non-updated code.
Assignee

Comment 1

a year ago
Posted patch WIP: Replace pixman regions (obsolete) — Splinter Review

Comment 2

a year ago
can bug 1369390 be duped here?
Assignee

Comment 3

a year ago
(In reply to Mayank Bansal from comment #2)
> can bug 1369390 be duped here?

Yeah, I seem to be forgetful. Sorry.
Assignee

Updated

a year ago
Duplicate of this bug: 1369390
Assignee

Comment 5

a year ago
Attachment #8953571 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8954063 - Attachment is obsolete: true

Comment 7

a year ago
mozreview-review
Comment on attachment 8955353 [details]
Bug 1440753: Replace pixman regions with our own region code.

https://reviewboard.mozilla.org/r/224532/#review230466


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/src/nsRegion.cpp:48
(Diff revision 1)
> -    boxes[i] = RectToBox(rect);
> +    newRegion.AddRect(rect);
>    }
>  
> -  pixman_region32_t region;
> -  // This will union all of the rectangles and runs in about O(n lg(n))
> -  pixman_region32_init_rects(&region, boxes, n);
> +  mBands.swap(newRegion.mBands);
> +  mBounds = newRegion.mBounds;
> +  return;

Warning: Redundant return statement at the end of a function with a void return type [clang-tidy: readability-redundant-control-flow]
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
Comment on attachment 8955353 [details]
Bug 1440753: Replace pixman regions with our own region code.

https://reviewboard.mozilla.org/r/224532/#review230500


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/src/nsRegion.cpp:48
(Diff revision 2)
> -    boxes[i] = RectToBox(rect);
> +    newRegion.AddRect(rect);
>    }
>  
> -  pixman_region32_t region;
> -  // This will union all of the rectangles and runs in about O(n lg(n))
> -  pixman_region32_init_rects(&region, boxes, n);
> +  mBands.swap(newRegion.mBands);
> +  mBounds = newRegion.mBounds;
> +  return;

Warning: Redundant return statement at the end of a function with a void return type [clang-tidy: readability-redundant-control-flow]

Comment 10

a year ago
mozreview-review
Comment on attachment 8955353 [details]
Bug 1440753: Replace pixman regions with our own region code.

https://reviewboard.mozilla.org/r/224532/#review231754

::: gfx/src/nsRegion.h:68
(Diff revision 2)
>  
> -  nsRegion () { pixman_region32_init(&mImpl); }
> -  MOZ_IMPLICIT nsRegion (const nsRect& aRect) { pixman_region32_init_rect(&mImpl,
> -                                                                          aRect.X(),
> -                                                                          aRect.Y(),
> -                                                                          aRect.Width(),
> +  nsRegion() { }
> +  MOZ_IMPLICIT nsRegion(const nsRect& aRect) {
> +    mBounds = aRect;
> +  }
> +  explicit nsRegion(mozilla::gfx::ArrayView<pixman_box32_t> aRects)

The callers of this function (looks to be just display list code) converted into the pixman_box32_t format especially for this, so converting them back is somewhat wasteful.

We should try fix those callers and change the signature here, either now, or file a follow-up.

The idea of this constructor was also that it would be quicker to initialize a region by providing the full list of rectangles rather than looping over Or().

It looks like this is now just identical to a loop over Or(), so it no longer adds value. Do you think this is still possible? Or should we just get rid of it?

::: gfx/src/nsRegion.h:99
(Diff revision 2)
>    friend std::ostream& operator<<(std::ostream& stream, const nsRegion& m);
> +  void OutputToStream(std::ostream& stream) const;
>  
>    void Swap(nsRegion* aOther)
>    {
> -    pixman_region32_t tmp = mImpl;
> +    mBands.swap(aOther->mBands);

Don't we need to swap mBounds here too?

::: gfx/src/nsRegion.h:123
(Diff revision 2)
> +    virtual void OutputOp() = 0;
> +  };
> +#endif
> +public:
> +
> +  void AssertState() const {

Maybe move the impl of this into the .cpp file? It's a big header already.

::: gfx/src/nsRegion.h:214
(Diff revision 2)
> +
> +    std::vector<Band> newBands;
> +
> +    while (true) {
> +      while (true) {
> +        while (idx != mBands.size() && mBands[idx].bottom <= aRegion.mBands[idxOther].top) {

Can you please add some more comments (here, and the other operations) about what the algorithm is doing.

Could possibly add some helpers for the common checks and inner loops too to give names to some of the pieces.

::: gfx/src/nsRegion.h:499
(Diff revision 2)
> +    while (idx < mBands.size()) {
> +      if (mBands[idx].top >= aRect.YMost()) {
> +        // This band is entirely after aRect. Done.
> +        mBounds = CalculateBounds();
> +        AssertState();
> +        return *this;

Could we move the loop into a helper to avoid needing this function exit boilerplate 3 times?

::: gfx/src/nsRegion.h:517
(Diff revision 2)
> +      }
> +
> +      // This band intersects with aRect.
> +
> +      if (mBands[idx].top < aRect.Y()) {
> +        // This band starts above the start of aRect.

Comments in this function make it much easier to follow!

Could still add something like 'This bands starts above the start of aRect, split the band into two along the intersection, and continue to the next iteration to process the one that now intersects exactly.'

::: gfx/src/nsRegion.h:1001
(Diff revision 2)
> +
> +    AssertState();
> +    EnsureSimplified();
> +  }
> +
> +  nsRect CalculateBounds() const

Some of the callers could probably have worked out the left/right extents as they've already walked the bands, and we could avoid needing to do it again.

Probably not worth doing now, but maybe a comment suggesting it if it ever shows up in profiles?

::: gfx/src/nsRegion.h:1048
(Diff revision 2)
> +      : top(aRect.Y()), bottom(aRect.YMost())
> +    {
> +      mStrips.push_back({ aRect.X(), aRect.XMost() });
> +    }
>  
> -  RectIterator RectIter() const { return RectIterator(*this); }
> +    void InsertStrip(const Strip& aStrip)

SubStrip has great comments, can we have them here too?

::: gfx/src/nsRegion.cpp:100
(Diff revision 2)
>  // by iterating over both rows simultaneously and adding up
>  // the additional increase in area caused by extending each
>  // of the rectangles to the combined height of both rows
> -static uint32_t ComputeMergedAreaIncrease(pixman_box32_t *topRects,
> -		                     pixman_box32_t *topRectsEnd,
> -		                     pixman_box32_t *bottomRects,
> +uint32_t
> +nsRegion::ComputeMergedAreaIncrease(const Band& aTopBand,
> +		                            const Band& aBottomBand)

alignment

::: gfx/src/nsRegion.cpp:168
(Diff revision 2)
> +  return totalArea;
>  }
>  
>  void nsRegion::SimplifyOutwardByArea(uint32_t aThreshold)
>  {
> -
> +  if (mBands.size() < 2) {

Can't we simplify within a single band?

::: gfx/src/nsRegion.cpp:243
(Diff revision 2)
> +        // State from this point on along the vertical edge:
> +        // 0 - Empty
> +        // 1 - Touched by top rect
> +        // 2 - Touched by bottom rect
> +        // 3 - Touched on both sides
> +        int state;

Can you use EnumSet<T> for this?

That way the logic below would look like:

state = oldState - TOUCHED_BY_TOP;
instead of
state = oldState ^ 1;

::: gfx/tests/gtest/TestRegion.cpp:209
(Diff revision 2)
> +    r.OrWith(nsRect(34, 112, 1, 12));
> +    r.OrWith(nsRect(75, 112, 51, 12));
> +    r.OrWith(nsRect(34, 124, 1, 10));
> +    r.OrWith(nsRect(75, 124, 44, 10));
> +    r.OrWith(nsRect(34, 134, 1, 19));
> +    r.OrWith(nsRect(22, 17, 96, 27));

Is there meant to be EXPECT_SOMETHING lines for each of these tests, or are you purely testing for crashing?

Seems like it'd be easy and valuable to test a pixel inside and a pixel outside the final region to see if it's somewhat sane.
Attachment #8955353 - Flags: review?(matt.woodrow) → review+
Assignee

Comment 11

a year ago
mozreview-review
Comment on attachment 8955353 [details]
Bug 1440753: Replace pixman regions with our own region code.

https://reviewboard.mozilla.org/r/224532/#review232016

::: gfx/src/nsRegion.h:68
(Diff revision 2)
>  
> -  nsRegion () { pixman_region32_init(&mImpl); }
> -  MOZ_IMPLICIT nsRegion (const nsRect& aRect) { pixman_region32_init_rect(&mImpl,
> -                                                                          aRect.X(),
> -                                                                          aRect.Y(),
> -                                                                          aRect.Width(),
> +  nsRegion() { }
> +  MOZ_IMPLICIT nsRegion(const nsRect& aRect) {
> +    mBounds = aRect;
> +  }
> +  explicit nsRegion(mozilla::gfx::ArrayView<pixman_box32_t> aRects)

It is, I wrote a patch to use RectAbsolute for this, however it got complicated and bitrotted fast, so indeed my plan is to do this in a follow-up.

::: gfx/src/nsRegion.h:99
(Diff revision 2)
>    friend std::ostream& operator<<(std::ostream& stream, const nsRegion& m);
> +  void OutputToStream(std::ostream& stream) const;
>  
>    void Swap(nsRegion* aOther)
>    {
> -    pixman_region32_t tmp = mImpl;
> +    mBands.swap(aOther->mBands);

Good catch.. I don't think this has any users now so I've just killed it. This was meant for usecases where we have std::move nowadays anyway.

::: gfx/src/nsRegion.h:123
(Diff revision 2)
> +    virtual void OutputOp() = 0;
> +  };
> +#endif
> +public:
> +
> +  void AssertState() const {

Sadly, we can't, what I did is add another function called AssertStateInternal and moved that into the CPP file (moving this one there would cause it not to get inlined in none region debug builds)

::: gfx/src/nsRegion.cpp:168
(Diff revision 2)
> +  return totalArea;
>  }
>  
>  void nsRegion::SimplifyOutwardByArea(uint32_t aThreshold)
>  {
> -
> +  if (mBands.size() < 2) {

Not with this algorithm, we only merge different bands with each other, that's what the old algorithm did so I left it that way.

::: gfx/src/nsRegion.cpp:243
(Diff revision 2)
> +        // State from this point on along the vertical edge:
> +        // 0 - Empty
> +        // 1 - Touched by top rect
> +        // 2 - Touched by bottom rect
> +        // 3 - Touched on both sides
> +        int state;

I'm all for the const int there.. but - .. really? that's like the worst operator overload for that like.. ever :p.

::: gfx/tests/gtest/TestRegion.cpp:209
(Diff revision 2)
> +    r.OrWith(nsRect(34, 112, 1, 12));
> +    r.OrWith(nsRect(75, 112, 51, 12));
> +    r.OrWith(nsRect(34, 124, 1, 10));
> +    r.OrWith(nsRect(75, 124, 44, 10));
> +    r.OrWith(nsRect(34, 134, 1, 19));
> +    r.OrWith(nsRect(22, 17, 96, 27));

Nope, this is testing for crashes (or internal assertions when DEBUG_REGIONS is on).

Sure.. I guess :-).
Comment hidden (mozreview-request)

Comment 13

a year ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9297cea0239
Replace pixman regions with our own region code. r=mattwoodrow

Comment 14

a year ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ccc12de326
Addendum: Initialize local bool to satisfy valgrind which somehow got upset. r=mattwoodrow
When this landed (comment 13), these performance regressions were noticed:

== Change summary for alert #12023 (as of Fri, 09 Mar 2018 02:27:36 GMT) ==

Regressions:

  6%  stylebench linux64 opt e10s stylo     64.59 -> 60.96
  5%  stylebench linux64 pgo e10s stylo     71.06 -> 67.41

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12023
Assignee

Comment 17

a year ago
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #16)
> When this landed (comment 13), these performance regressions were noticed:
> 
> == Change summary for alert #12023 (as of Fri, 09 Mar 2018 02:27:36 GMT) ==
> 
> Regressions:
> 
>   6%  stylebench linux64 opt e10s stylo     64.59 -> 60.96
>   5%  stylebench linux64 pgo e10s stylo     71.06 -> 67.41
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=12023

This regression looks real, I've found the reason for the bug it was backed out for, but I'll wait with relanding until I know more about this.
Flags: needinfo?(bas)
Assignee

Comment 18

a year ago
Did these improve again with the backout, btw?
(In reply to Bas Schouten (:bas.schouten) from comment #18)
> Did these improve again with the backout, btw?

Yes, the backout canceled the regressions.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

a year ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c652b6d362f6
Replace pixman regions with our own region code. r=mattwoodrow

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c652b6d362f6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Updated

a year ago
Depends on: 1448301

Updated

a year ago
Depends on: 1448325
Duplicate of this bug: 1444332
When this landed in comment 22, we noticed small perf:

== Change summary for alert #12341 (as of Thu, 22 Mar 2018 20:17:25 GMT) ==

Improvements:

  2%  tp6_google linux64 opt 1_thread e10s stylo     434.67 -> 425.12

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12341
Assignee

Updated

a year ago
Flags: needinfo?(bas)
Comment hidden (mozreview-request)

Comment 28

a year ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85b5825a721
Replace pixman regions with our own region code. r=mattwoodrow
(In reply to Cosmin Sabou [:cosmin_s] from comment #25)
> Backed out for permafailing on layout/reftests/scrolling/iframe-scrolling-attr-2.html a=backout 

This ^ permafail seems to still be a problem -- it came back in today's landing (on OS X 10.10 opt):
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d85b5825a721eb18febaf5b9e068a9ec3268a2f5

In the failures I clicked through, the failure snapshots show that there are only 3 "X" characters in the upper-left section, while the reference case has 4 "X" characters.

(I believe a backout is coming shortly; sheriffs are sorting through a few unrelated issues simultaneously).

Comment 30

a year ago
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a78edaef5e7
Backed out changeset d85b5825a721 for frequent OSX iframe-scrolling-attr-2.html failures on a CLOSED TREE.
Backed out changeset d85b5825a721 (bug 1440753) for frequent OSX iframe-scrolling-attr-2.html failures on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/3a78edaef5e7d49c3032b1219415a76dfdded206

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d85b5825a721eb18febaf5b9e068a9ec3268a2f5

Log link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d85b5825a721eb18febaf5b9e068a9ec3268a2f5

Log snippet: 11:32:07     INFO - REFTEST TEST-START | file:///Users/cltbld/tasks/task_1522691799/build/tests/reftest/tests/layout/reftests/scrolling/iframe-scrolling-attr-2.html == file:///Users/cltbld/tasks/task_1522691799/build/tests/reftest/tests/layout/reftests/scrolling/iframe-scrolling-attr-ref.html
11:32:07     INFO - REFTEST TEST-LOAD | file:///Users/cltbld/tasks/task_1522691799/build/tests/reftest/tests/layout/reftests/scrolling/iframe-scrolling-attr-2.html | 35 / 97 (36%)
11:32:07     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/tasks/task_1522691799/build/tests/reftest/tests/layout/reftests/scrolling/iframe-scrolling-attr-2.html == file:///Users/cltbld/tasks/task_1522691799/build/tests/reftest/tests/layout/reftests/scrolling/iframe-scrolling-attr-ref.html | image comparison, max difference: 255, number of differing pixels: 80
Flags: needinfo?(bas)
Assignee

Comment 32

a year ago
(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #29)
> (In reply to Cosmin Sabou [:cosmin_s] from comment #25)
> > Backed out for permafailing on layout/reftests/scrolling/iframe-scrolling-attr-2.html a=backout 
> 
> This ^ permafail seems to still be a problem -- it came back in today's
> landing (on OS X 10.10 opt):
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=d85b5825a721eb18febaf5b9e068a9ec3268a2f5
> 
> In the failures I clicked through, the failure snapshots show that there are
> only 3 "X" characters in the upper-left section, while the reference case
> has 4 "X" characters.
> 
> (I believe a backout is coming shortly; sheriffs are sorting through a few
> unrelated issues simultaneously).

Yeah. I saw them on some of my old pushes. On my latest pushes five retries didn't repro it. I suspect it's some kind of timing issue. There appears to be an old intermittent for this test as well. I wonder of there's something I can do to make the test more reliable. I suspect it's not actually a bug in my patch but it's tricky to confirm
Flags: needinfo?(bas)
Assignee

Comment 33

a year ago
Daniel, you looked at this. Out of curiosity, do you think it's possible we're measuring checkerboarding here? My patch could definitely affect timing.
Flags: needinfo?(dholbert)
(In reply to Bas Schouten (:bas.schouten) from comment #32)
> Yeah. I saw them on some of my old pushes. On my latest pushes five retries
> didn't repro it.

Interesting! It seems to have been pretty reliable on the mozilla-inbound push, for whatever reason (18/19 attempts hit the issue).

> There appears to be an old intermittent for this test as well.

That intermittent (bug 1435375) seems unrelated -- it's Android-only, and in its failure screenshots, it seems like we're simply failing to do any 'overflow' clipping at all (or something like that).

(In reply to Bas Schouten (:bas.schouten) from comment #33)
> Daniel, you looked at this. Out of curiosity, do you think it's possible
> we're measuring checkerboarding here? My patch could definitely affect
> timing.

I don't have recent familiarity with scrolling/checkerboarding code, but that seems plausible.

(You could test that theory by comparing a try push of just your patch vs. your patch with "reftest-wait" and  setTimeout(100,...) in this test.  If the unmodified patch has a significantly higher failure rate vs. the setTimeout version, then that'd indicate a reftest-snapshotting vs. checkerboarding race of some sort.)
Flags: needinfo?(dholbert)

Comment 35

a year ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9db3f332a19
Replace pixman regions with our own region code. r=mattwoodrow

Comment 36

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9db3f332a19
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Updated

a year ago
Depends on: 1451597

Comment 37

a year ago
Backout by nerli@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/1b258f938525
Backed out changeset c9db3f332a19 for content sometimes not being rendered (bug 1451597). a=backout

Updated

a year ago
Depends on: 1451688
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

a year ago
Target Milestone: mozilla61 → ---
Flags: needinfo?(bas)
Target Milestone: --- → mozilla61
Target Milestone: mozilla61 → ---

Comment 39

a year ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2926745a0fee
Replace pixman regions with our own region code. r=mattwoodrow

Comment 40

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2926745a0fee
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee

Updated

a year ago
Flags: needinfo?(bas)

Updated

11 months ago
Depends on: 1471714
Depends on: 1537660
You need to log in before you can comment on or make changes to this bug.