1129871 - Avoid needlessly copying tiles

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

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.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Simplify TiledContentClient a bit.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 3

[Nicolas Silva [:nical]]

(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

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(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...

Comment 5

[Nicolas Silva [:nical]]

Attachment: Remove some more copying around

Comment 6

[Benoit Girard (:BenWa)]

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.

Comment 7

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8a74ba8122
https://hg.mozilla.org/integration/mozilla-inbound/rev/b57d930e0ba6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Nicolas Silva [:nical]]

fixed and relanded
https://hg.mozilla.org/integration/mozilla-inbound/rev/6952d9fe4f36
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1bb91f60697

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6952d9fe4f36
https://hg.mozilla.org/mozilla-central/rev/e1bb91f60697

Comment 11

[Carsten Book [:Tomcat]]

backed out for causing bug 1133484

Comment 12

[Nicolas Silva [:nical]]

I'll get back to this soon, focusing other things at the moment.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e88a24cd39

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e0e88a24cd39