Closed Bug 1440753 Opened 6 years ago Closed 6 years ago

Current Pixman region code shows up in profiles a lot

Categories

(Core :: Graphics, enhancement, P3)

Product:
Core
Component:
Graphics
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

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

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1440753: Replace pixman regions with our own region code.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details 
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c652b6d362f6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox61:
          --- → fixed
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c9db3f332a19
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago

          status-firefox61:
          --- → fixed
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

          status-firefox61:
          fixed → ---
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2926745a0fee
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago

          status-firefox61:
          --- → fixed
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

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: