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)
Core
Graphics
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.
Assignee | ||
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
can bug 1369390 be duped here?
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Mayank Bansal from comment #2)
> can bug 1369390 be duped here?
Yeah, I seem to be forgetful. Sorry.
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8953571 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8954063 -
Attachment is obsolete: true
Comment 7•7 years 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(®ion, 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•7 years 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(®ion, 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]
Updated•7 years ago
|
Priority: -- → P3
Comment 10•7 years 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•7 years 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•7 years 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•7 years 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
Comment 15•7 years ago
|
||
Backed out 2 changesets (bug 1440753) for talos bustages
Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=166938491&repo=mozilla-inbound
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14ef02ecc3033a4fa1a28d23897acc5239b620ea
Pushes that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=75ccc12de3267a8876946fc96f22189b7ef5ba32
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d9297cea0239855bcbef78e40967b4054983a093
Flags: needinfo?(bas)
Comment 16•7 years ago
|
||
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•7 years 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•7 years ago
|
||
Did these improve again with the backout, btw?
Comment 19•7 years ago
|
||
(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•7 years 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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 25•7 years ago
|
||
Backed out for permafailing on layout/reftests/scrolling/iframe-scrolling-attr-2.html a=backout
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=cfefdc3139bc24c5698dae56b2d7dc3ee2951a0a&fromchange=c652b6d362f6d3cba13cd3f25c5c878847fe8dac&selectedJob=169853136
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=169853136&repo=mozilla-inbound&lineNumber=53338
Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc910c1f477d8095dcde2c96b7e8639211b9a84
Status: RESOLVED → REOPENED
status-firefox61:
fixed → ---
Flags: needinfo?(bas)
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
Comment 26•7 years ago
|
||
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•7 years ago
|
Flags: needinfo?(bas)
Comment hidden (mozreview-request) |
Comment 28•7 years 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
Comment 29•7 years ago
|
||
(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•7 years 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.
Comment 31•7 years ago
|
||
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•7 years 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•7 years 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)
Comment 34•7 years ago
|
||
(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•7 years 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•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 37•7 years 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
Comment 38•7 years ago
|
||
Backed out changeset c9db3f332a19 (bug 1440753) for content sometimes not being rendered (bug 1451597). a=backout
Push with the issue: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=071ee904485e21e19ca08456d32bce6825b77a26&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=running&filter-searchStr=nightly
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=93fda7f5d1f8e66663e8adcd36cb17ac90c1c938&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=running&filter-searchStr=nightly
Updated•7 years ago
|
Updated•7 years ago
|
Target Milestone: mozilla61 → ---
Updated•7 years ago
|
Flags: needinfo?(bas)
Target Milestone: --- → mozilla61
Updated•7 years ago
|
Target Milestone: mozilla61 → ---
Comment 39•7 years 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•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bas)
You need to log in
before you can comment on or make changes to this bug.
Description
•