Closed Bug 110979 Opened 22 years ago Closed 20 years ago
Item should cache micro-tiles
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+
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?
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?
Place in layout/svg/base/src
Attachment #58702 - 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 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.
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.
The libart rendering backend checked in as part of bug#182533 now encapsulates utas in nsISVGRenderRegion objects and caches them where appropriate. Marking fixed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.