XPComify public mdb interfaces, and remove orkin layer from Mork

VERIFIED FIXED

Status

MailNews Core
Database
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Assignee)

Description

16 years ago
This bug is to track the work of making Mdb+Mork support XPCOM, and get rid of
orkin layer of Mork. The orkin objects only reasons for being were to support
xpcom ref-counting semantics without requiring the caller to pass in an
environment, and to implement the MDB interfaces.  The change is to make the
underlying mork objects directly implement MDB interfaces, and to inherit from
nsISupports instead of nsIMdbSupports.  Most of the work is in Mork, but there
are some changes in mailnews/base and mailnews/db, and xpfe/components/history
(because CloseMdbObject goes away).
This fix should reduce some memory bloat, since we won't need the orkin objects,
and it fixes a lot of memory leaks because the mork strong and weak ref-counting
semantics made it so that many objects were never released. The code should be
easier to maintain as well, now that an extra layer is gone.
(Assignee)

Comment 1

16 years ago
I'll start attaching patches now. I've been running with this for several days
without a problem (under purify as well) but I need to go over the code some
more to make sure I haven't broken anything.
Status: NEW → ASSIGNED
QA Contact: esther → stephend
(Assignee)

Comment 2

16 years ago
Created attachment 58437 [details] [diff] [review]
changes to mdb, the public interface file

Most of these changes are to use NS_IMETHOD explicitly, and inherit from
nsISupports. I've also removed some unused interfaces. I'd appreciate it if you
guys could start reviewing because it's going to take a while :-(
(Assignee)

Comment 3

16 years ago
Created attachment 58439 [details] [diff] [review]
remove unused mork files.

A couple of these have never been used, but most of the orkin ones will become
unused when the rest of the changes get checked in.
(Assignee)

Comment 4

16 years ago
Created attachment 58440 [details] [diff] [review]
changes to go along with mork changes

Basically, CloseMdbObject/CutStrongRef(mEnv) -> Release
(Assignee)

Comment 5

16 years ago
Alec, would you r or sr the history changes? Thx.
(Assignee)

Comment 6

16 years ago
Created attachment 58441 [details] [diff] [review]
corresponding changes for folder cache.

Use Release() directly.

Comment 7

16 years ago
Comment on attachment 58440 [details] [diff] [review]
changes to go along with mork changes

bienvenu you are a monster!
sr=alecf
Attachment #58440 - Flags: superreview+

Comment 8

16 years ago
Comment on attachment 58437 [details] [diff] [review]
changes to mdb, the public interface file

r=naving
Attachment #58437 - Flags: review+

Comment 9

16 years ago
Comment on attachment 58439 [details] [diff] [review]
remove unused mork files.

r=naving
Attachment #58439 - Flags: review+

Comment 10

16 years ago
Comment on attachment 58440 [details] [diff] [review]
changes to go along with mork changes

r=naving
Attachment #58440 - Flags: review+

Comment 11

16 years ago
Comment on attachment 58441 [details] [diff] [review]
corresponding changes for folder cache.

r=naving
Attachment #58441 - Flags: review+

Comment 12

16 years ago
Good job david, that was easy to review. So do you see the size of mork.dll go
up or down and by what.

Currently mork.dll has size = 683736 bytes. This is from purify opt build (a few 
days old). 



(Assignee)

Comment 13

16 years ago
Created attachment 58500 [details] [diff] [review]
mork diffs to get rid of orkin layer

OK, this is the hard part. I took all the orkin methods and moved them into
their mork counterparts, and changed them to work as methods of the mork
classes. Sorry for the volume of the changes :-(
(Assignee)

Comment 14

16 years ago
Created attachment 58533 [details] [diff] [review]
address book diffs

coresponding address book diffs, plus changes to force closed any open address
books at shutdown.
(Assignee)

Comment 15

16 years ago
I don't know how much smaller this will make an optimized build of mork.dll - I
would expect a small decrease (maybe 3-5%) but that's just a guess. I removed a
few unused classes, but most of the code was just moved from ork to mork. I
expect the runtime savings to be in the same range, but the big change will be
the reduction of leaks when switching between folders, which will cut down the
bloat numbers quite a bit, I believe.
(Assignee)

Comment 16

16 years ago
Created attachment 58596 [details] [diff] [review]
msgdb changes

msg db changes, mostly just using AddRef and Release

Comment 17

16 years ago
Comment on attachment 58500 [details] [diff] [review]
mork diffs to get rid of orkin layer

Could you explain why you changed size here ?
-  inSize += 8; // sizeof(mork_u4) * 2
+  inSize += 12; // sizeof(mork_u4) * 3


+mork_refs
+morkTable::AddStrongRef(morkEnv *ev)
+{
+  return (mork_refs) AddRef();
+}
+
how is *ev used here? could you explain more 

+mork_refs
+morkTable::CutStrongRef(morkEnv *ev)
+{
+  return (mork_refs) Release();
+}

when moving methods from 
orkin layer you chose not
to implement certain methods, why did you do
that ?
(Assignee)

Comment 18

16 years ago
the change in size has to do with how I had to tweak the HEAP_STATS debugging
code (the code you're asking about is not turned on by default, either in debug
or release builds, if that helps). I needed to add the heap object to the
allocated blocks so that when I freed them, I could know which heap they were
getting freed from and adjust the various heap stats debug counts for that heap.

The ev is not used, but when you're overriding a virtual method like
AddStrongRef, you need to have the same argument list as the overridden virtual
method. Why did I have to override those methods? I had to do that so that that
object used xpcom ref-counting semantics instead of the mork ref-counting
semantics. I couldn't change the call site because it's called from a
polymorphic method that gets called on lots of internal objects. Clear as mud,
right? :-(

Comment 19

16 years ago
Comment on attachment 58533 [details] [diff] [review]
address book diffs

r=naving

you can remove null 
assignments after release

+      m_mdbEnv->Release();
       m_mdbEnv = nsnull;


+		m_mdbStore->Release();
		m_mdbStore = nsnull;
Attachment #58533 - Flags: review+
(Assignee)

Comment 20

16 years ago
I could remove the = null in the destructor, but I definitely can't remove the
second one with the store!

Comment 21

16 years ago
Comment on attachment 58596 [details] [diff] [review]
msgdb changes

r=naving
you can remove null assignments after release
Attachment #58596 - Flags: review+

Comment 22

16 years ago
Comment on attachment 58500 [details] [diff] [review]
mork diffs to get rid of orkin layer

r=naving

thanks for explaining it.

Could you double check for
the methods that were not 
implemented.
Attachment #58500 - Flags: review+
(Assignee)

Comment 23

16 years ago
thanks for reviewing it. I'm pretty confident the unimplemented methods are
correct and not called because I would see assertions.
sr=sspitzer on all patches.
(Assignee)

Comment 25

16 years ago
fixes checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
This landed, and I'll be verifying the separate orkin leaks...

Verified FIXED.
Status: RESOLVED → VERIFIED

Comment 27

16 years ago
bienvenu, wanna take a look at this, please ?

From "nebiros" tinderbox:
-- snip --
"/usr/include/memory.h", line 21: Error: Only one of a set of overloaded
functions can be extern "C".
"/usr/include/memory.h", line 21: Error: std::memchr(const void*, int, unsigned
long), returning void*, was previously declared returning const void*.
gmake[5]: *** [orkinHeap.o] Error 2
gmake[5]: Leaving directory
`/builds/tinderbox/SeaMonkey/SunOS_5.7_Depend/mozilla/obj-sparc-sun-solaris2.7/db/mork/src'
gmake[4]: *** [libs] Error 2
-- snip --
(Assignee)

Comment 28

16 years ago
I have been looking, and I have no idea what's going on. If you have any idea
what's going on, let me know.

Comment 29

16 years ago
I filed bug 111574 ("WS6U2 tinderbox burning due broken
db/mork/src/orkinHeap.cpp") for that issue ...
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.