Last Comment Bug 99151 - nsIMemory needs to be frozen
: nsIMemory needs to be frozen
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: mozilla1.0
Assigned To: Doug Turner (:dougt)
: Scott Collins
Mentors:
Depends on:
Blocks: 98278
  Show dependency treegraph
 
Reported: 2001-09-10 14:10 PDT by Doug Turner (:dougt)
Modified: 2001-10-25 17:16 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed Patch v.1 (8.68 KB, patch)
2001-10-22 17:17 PDT, Doug Turner (:dougt)
brendan: superreview+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2001-09-10 14:10:45 PDT
 
Comment 1 Doug Turner (:dougt) 2001-10-22 17:17:27 PDT
Created attachment 54585 [details] [diff] [review]
Proposed Patch v.1
Comment 2 Alec Flett 2001-10-23 11:12:32 PDT
I guess I'm cool with this, but lets get some other people into the mix...
adding brendan on the hopes that he adds other interested parties..
Comment 3 Patrick C. Beard 2001-10-23 19:03:46 PDT
XPCOM/OJI plugins expect nsIMemory to be frozen.
Comment 4 Brendan Eich [:brendan] 2001-10-23 20:17:44 PDT
Looks like dougt's patch doesn't change anything in the interface itself. 
That's good, given beard's comment.  I never thought nsIMemory needed fixing (or
needed renaming from nsIAllocator, for that matter -- warren killed himself and
toasted the trees landing the change).

The money bug is the one about avoiding going through nsIMemory or its impl's
indirection all the time.  Do we believe we're better off with this ability (not
tested, probably broken) to field-upgrade our allocator?

/be
Comment 5 Doug Turner (:dougt) 2001-10-24 06:33:05 PDT
That bug is:  http://bugzilla.mozilla.org/show_bug.cgi?id=75620

if you get a nsIMemory, you may have to pay a penalty.  The whole point of this
interface is to ensure that you are using the same allocator in all of the places.  

We may be able to speed up the static helper class nsMemory (see
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=47375), but it can not
be inlined for the same reasons above.  

The optimal solution is to use the system alloc in places where they do not have
to pass buffers between interface.
Comment 6 Brendan Eich [:brendan] 2001-10-24 11:17:07 PDT
doug, thanks for the bug cite (I commented there).  I don't care whether OJI and
plugins pay XPCOM costs, but I would bet we're already using a bunch of
different direct hooks on malloc (mostly via NSPR and nsCRT) -- so who are we
kidding about field-upgrades?  malloc is malloc is malloc.  Weak symbols help us
on some OSes, should we want to wrap malloc.

It's hard to bound access to memory in some parts of our system, such that we
can be sure everyone who could get his hands on an allocation is within a cloud
of code that doesn't expose that memory via XPCOM interfaces.

I'm pretty sure we're paying a tax here for no benefit.  Comments from others
are more than welcome -- am I swerving too hard against this bit of COM purity?
/be
Comment 7 Doug Turner (:dougt) 2001-10-25 10:24:26 PDT
Brendan, this bug is about freezing the nsIMemory interface which is required by
embedders and plugin's.  This bug is not about fixing users of nsIMemory to use
raw malloc or another direct call, it is about making an interrface available
that allows components to pass memory around without regard for allocator.

Comment 8 Brendan Eich [:brendan] 2001-10-25 14:22:12 PDT
Comment on attachment 54585 [details] [diff] [review]
Proposed Patch v.1

  * A client that wishes to be notified of low memory situations (for
  * example, because the client maintains a large memory cache that
  * could be released when memory is tight) may register with the


"should", not "may"

- * observer service (see nsIObserverService) using the
- * NS_MEMORY_PRESSURE_TOPIC ("memory-pressure") as the topic for
+ * observer service (see nsIObserverService) using the topic for
  * observation.

"passing the <<relevant>> topic for observation."

What should <<relevant>> be?  If "memory-pressure", say so in this sentence.
I think the sentence lost something crucial.

+ * Topics for Observeration


Observation misspelled.

I realize you just moved comments for the topic #defines (now gone) up into the big
doc-comment, but how about defining terms like "the flusher"?  Or juts change
"flusher" to "pressure observer"?

Fix these and sr=brendan@mozilla.org
Comment 9 Doug Turner (:dougt) 2001-10-25 17:16:49 PDT
fix checked in.  

Checking in content/xbl/src/nsXBLService.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLService.cpp,v  <--  nsXBLService.cpp
new revision: 1.133; previous revision: 1.132
done
Checking in intl/strres/src/nsStringBundle.cpp;
/cvsroot/mozilla/intl/strres/src/nsStringBundle.cpp,v  <--  nsStringBundle.cpp
new revision: 1.112; previous revision: 1.111
done
Checking in modules/libjar/nsJAR.cpp;
/cvsroot/mozilla/modules/libjar/nsJAR.cpp,v  <--  nsJAR.cpp
new revision: 1.85; previous revision: 1.84
done
Checking in xpcom/base/nsIAllocator.h;
/cvsroot/mozilla/xpcom/base/nsIAllocator.h,v  <--  nsIAllocator.h
new revision: 1.15; previous revision: 1.14
done
Checking in xpcom/base/nsIMemory.idl;
/cvsroot/mozilla/xpcom/base/nsIMemory.idl,v  <--  nsIMemory.idl
new revision: 1.7; previous revision: 1.6
done
Checking in xpcom/base/nsMemoryImpl.cpp;
/cvsroot/mozilla/xpcom/base/nsMemoryImpl.cpp,v  <--  nsMemoryImpl.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in xpcom/base/nsMemoryImpl.h;
/cvsroot/mozilla/xpcom/base/nsMemoryImpl.h,v  <--  nsMemoryImpl.h
new revision: 1.5; previous revision: 1.4
done

Note You need to log in before you can comment on or make changes to this bug.