Closed Bug 110979 Opened 23 years ago Closed 21 years ago

SVGRenderItem should cache micro-tiles


(Core :: SVG, defect)

Not set





(Reporter: jmt, Assigned: alex)




(2 files, 6 obsolete files)

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.
Needs the following header file in layout/svg/base/src too
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...
Attachment #58537 - Flags: needs-work+
Ever confirmed: true
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?
Attachment #58537 - Attachment is obsolete: true
Attachment #58536 - Attachment is obsolete: true
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(.)
Do you really want an opertor void*, as opposed to .get_address() or something?
Attachment #58701 - Attachment is obsolete: true
Place in layout/svg/base/src
Attachment #58702 - Attachment is obsolete: true
Attachment #58854 - Attachment is obsolete: true
I just checked this in on SVG_0_9_6_BRANCH.
Our development branch, SVG_20010721_BRANCH, is currently broken :-(
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 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.
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'.
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'.

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?
Yeah, sounds fine.
Depends on: svgbranch
The libart rendering backend checked in as part of bug#182533 now encapsulates
utas in nsISVGRenderRegion objects and caches them where appropriate. Marking fixed.
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.