Closed Bug 1129871 Opened 5 years ago Closed 4 years ago

Avoid needlessly copying tiles

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: nical, Assigned: nical)

Details

Attachments

(2 files)

We tend to forget about what's happening during copies and destruction of tile structures which are sort of POD but not really (they have a destructor). There's a ton of misleading things that can happen when mutating the copy of a tile (like discarding buffers) rather than the object we keep for the next step. So let's just remove this useless thinking overhead and mutate tiles in-place rather than passing copies around.
Attachment #8559760 - Flags: review?(jmuizelaar)
Comment on attachment 8559760 [details] [diff] [review]
Simplify TiledContentClient a bit.

Review of attachment 8559760 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have any problem with this other than my standard hate for the implicit reference taking of C++. The original decision here was made by bgirard so you may want to run it by him as well.
Attachment #8559760 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> I don't have any problem with this other than my standard hate for the
> implicit reference taking of C++. The original decision here was made by
> bgirard so you may want to run it by him as well.

I can turn these references into pointers if you prefer
(In reply to Nicolas Silva [:nical] from comment #3)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> > I don't have any problem with this other than my standard hate for the
> > implicit reference taking of C++. The original decision here was made by
> > bgirard so you may want to run it by him as well.
> 
> I can turn these references into pointers if you prefer

References are what we're supposed to use...
Attachment #8559832 - Flags: review?(bgirard)
Attachment #8559760 - Flags: review?(bgirard)
Comment on attachment 8559760 [details] [diff] [review]
Simplify TiledContentClient a bit.

Review of attachment 8559760 [details] [diff] [review]:
-----------------------------------------------------------------

This was designed like this explicitly because instead of doing fancy double buffering the tiling could would just replace the tiles that it needed. However we don't use the tiles like this anymore so it's fine if we remove this.
Attachment #8559760 - Flags: review?(bgirard) → review+
Attachment #8559832 - Flags: review?(bgirard) → review+
Backed out for gtest asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6112488026a

https://treeherder.mozilla.org/logviewer.html#?job_id=6312428&repo=mozilla-inbound

09:59:33 INFO - [3457] ###!!! ASSERTION: Unexpected placeholder tile - failed to allocate?: '!IsPlaceholder(newTile)', file /builds/slave/m-in-l64-st-an-d-0000000000000/build/src/gfx/layers/TiledLayerBuffer.h, line 537
09:59:33 INFO - Hit MOZ_CRASH() at /builds/slave/m-in-l64-st-an-d-0000000000000/build/src/memory/mozalloc/mozalloc_abort.cpp:37
https://hg.mozilla.org/mozilla-central/rev/6952d9fe4f36
https://hg.mozilla.org/mozilla-central/rev/e1bb91f60697
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
backed out for causing bug 1133484
Status: RESOLVED → REOPENED
Flags: needinfo?(nical.bugzilla)
Resolution: FIXED → ---
I'll get back to this soon, focusing other things at the moment.
Flags: needinfo?(nical.bugzilla)
https://hg.mozilla.org/mozilla-central/rev/e0e88a24cd39
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla38 → mozilla42
You need to log in before you can comment on or make changes to this bug.