Closed Bug 1440753 Opened 7 years ago Closed 7 years ago

Current Pixman region code shows up in profiles a lot

Categories

(Core :: Graphics, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

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

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch WIP: Replace pixman regions (obsolete) — Splinter Review
can bug 1369390 be duped here?
(In reply to Mayank Bansal from comment #2) > can bug 1369390 be duped here? Yeah, I seem to be forgetful. Sorry.
Attached patch WIP: Replace pixman regions v2 (obsolete) — Splinter Review
Attachment #8953571 - Attachment is obsolete: true
Attachment #8954063 - Attachment is obsolete: true
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 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 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+
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 :-).
Pushed by bschouten@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9297cea0239 Replace pixman regions with our own region code. r=mattwoodrow
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
(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)
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.
Pushed by bschouten@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c652b6d362f6 Replace pixman regions with our own region code. r=mattwoodrow
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1448301
Depends on: 1448325
Depends on: 1448333
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
Flags: needinfo?(bas)
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).
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)
(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)
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)
Pushed by bschouten@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c9db3f332a19 Replace pixman regions with our own region code. r=mattwoodrow
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1451597
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
Depends on: 1451688
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
Flags: needinfo?(bas)
Target Milestone: --- → mozilla61
Pushed by bschouten@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2926745a0fee Replace pixman regions with our own region code. r=mattwoodrow
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: needinfo?(bas)
Depends on: 1471714
Depends on: 1537660
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: