110979 - SVGRenderItem should cache micro-tiles

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[James Turner]

The handling of libArt microtile arrays (utas) by the SVG code is extremely
inefficent. Every SVGRenderItem calls svp_to_uta on any invalidate call, which
is  expensive. The following patch caches the microtiles, and also implements a
reference-counting wrapper around them to avoid uncessary allocations, copying
and free when passing them through nsISVGFrame::InvalidateRegion.

Comment 1

[James Turner]

Attachment: Share and cache microtiles in the SVG code

Needs the following header file in layout/svg/base/src too

Comment 2

[James Turner]

Attachment: Ref-counting wrapper class for libArt structures

Comment 3

[Bradley Baetz (:bbaetz)]

Comment on attachment 58537 [details]
Ref-counting wrapper class for libArt structures

This uses templates. Templates are good. Compilers are bad. You want nsRefPtr,
which won't work yet...

Comment 4

[Bradley Baetz (:bbaetz)]

actually, this is bad anyway, becausse you;re calling art_free rather than
art_uta free, however:

dbaron? Is it going to be possible to use an nsrefptr with a custom deallocator?
The stuff in the tree uses delete - are we going to need a wrapper class?

Comment 5

[James Turner]

Attachment: updated version of the patch, doesn't use templates

Comment 6

[James Turner]

Attachment: specialised the template 'by hand' for ArtUta

Comment 7

[Alex Fritze]

The patch looks good to me.

A few things:
- You mix tabs & spaces. Please convert everything to spaces. We use 2/tab.
- the new file (nsArtUtaRef.h ?) needs a license header.
- Please convert your member variable names from _foo to mFoo.
- I guess Accumulate(.) should be renamed to Combine(.) or something like that, 
  since it doesn't accumulate anymore.
- There are still occurences of ArtUta's in nsSVGForeignObjectFrame.cpp. Could
  you change them to using your ref class as well, please?
- Please be defensive when passing null-values into art_ functions. Especially
  for nSVGOuterSVGFrame::InvalidateRegion(.) you changed the logic such that it 
  might now get called with a null uta. We should have a guard clause here, so 
  that we don't pass null into art_rect_list_from_uta(.)

Comment 8

[Bradley Baetz (:bbaetz)]

Do you really want an opertor void*, as opposed to .get_address() or something?

Comment 9

[James Turner]

Attachment: incorporates all of Alex's suggestions

Comment 10

[James Turner]

Attachment: nsArtUtaRef.h, with Alex's suggestions taken into account

Place in layout/svg/base/src

Comment 11

[Alex Fritze]

Attachment: james' patch with tabs converted to spaces

Comment 12

[Alex Fritze]

Attachment: layout/svg/base/src/nsArtUtaRef.h with whitespace and license fixed

Comment 13

[Alex Fritze]

I just checked this in on SVG_0_9_6_BRANCH.
Our development branch, SVG_20010721_BRANCH, is currently broken :-(

Comment 14

[Bradley Baetz (:bbaetz)]

I still strongly disagree with the operator void*, returning 'this'. It should
be an operator ArtUta*, returning nsnull or mCounter->mInner.

'ArtUta& operator*()' vs 'const ARTuta& operator*() const' will not do what you
expect, and should warn on linux, in fact - see
http://gcc.gnu.org/ml/gcc/1999-06n/msg00655.html amongst others. Later gcc
versions (ie >=3.0) have this warning toned down for conversion operators, but
the behaviour is still correct. mozilla code is generally const-unsafe, because
of the idl generation.

your int mRefCount should be an nsrefcnt instead, as well, I think. (Its
probably the same type, though, if you poke through the typedefs)

What happens to the refcount after:

nsArtUtaRef foo(...); *foo = x;

?

You should also have a default constructor which implies a null pointer being given.

See the impl of nsRefPtr in xpcom/base/nsAutoPtr.h. This is currently not used
because of the template stuff, and we don't want exactly that.

Comment 15

[Alex Fritze]

We're leaking micro-tile arrays like a sieve, so it'd be nice to get this code 
in soon.

1. Why do we need an operator 'void*' at all? 

2. Even though it might not be clear in some cases which method will be 
selected, it still makes sense to have 'const ARTuta& operator*() const' for 
those occasions where you just have a 'const nsArtUtaRef'.

Comment 16

[Alex Fritze]

1. OK, so we need 'operator void*()' for 'if (!artref)'-style expressions. I'd 
consider an 'operator ArtUta*()' dangerous. This smartpointer class is designed 
to replace raw ArtUta* usage; having a conversion operator here that allows 
transparent substitution of smartpointers for raw pointers is counterproductive.
Personally I'm ok about 'void*()'. A poor man's substitute might be 'operator 
bool()'. Or we could explicitley use 'if (!artref.get())'. What do you think?

2. Since we don't use it, why don't we just do away with the 'const ARTuta& 
operator*() const'.

Comment 17

[James Turner]

I thought this was already checked in!
I'd much rather have if (!utaref.get()) and get rid of the void* operator
if we do that, and swap the int type for nsrefcnt, are you okay with this, bbaetez?

Comment 18

[Bradley Baetz (:bbaetz)]

Yeah, sounds fine.

Comment 19

[Alex Fritze]

The libart rendering backend checked in as part of bug#182533 now encapsulates
utas in nsISVGRenderRegion objects and caches them where appropriate. Marking fixed.