Sergey Yanovich Assignee
	Description • 16 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008092816 Iceweasel/3.0.3 (Debian-3.0.3-3)
Build Identifier: trunk

nsComponentManagerImpl::HashContractID() is only called when mMon is already entered, so its re-entrance is probably excessive. Removing this should slightly reduce Ts.

Reproducible: Always




Patch follows.

	Sergey Yanovich Assignee
	Comment 1 • 16 years ago

Patch removes |nsAutoMonitor| from  |nsComponentManagerImpl::HashContractID()|.

	Benjamin Smedberg
	Comment 2 • 16 years ago

Comment on attachment 345687 [details] [diff] [review]
patch v1.0

The patch looks correct, but please document in the header that this method must only be called while the monitor is held (and ideally assert this is true, but I don't think NSPR has that capability currently)

	Sergey Yanovich Assignee
	Comment 3 • 16 years ago

Patch:
* adds a comment in xpcom/components/nsComponentManager.h;
* adds an assertion using PR_GetMonitorEntryCount().

	Benjamin Smedberg
	Comment 4 • 16 years ago

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

thanks!

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 5 • 16 years ago

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

We'll get this after beta 2

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 6 • 16 years ago

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

a191=beltzner who likes Ts wins

	Serge Gautherie (:sgautherie)
	Comment 7 • 16 years ago

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

http://hg.mozilla.org/mozilla-central/rev/8b5a38ba459a

	Serge Gautherie (:sgautherie)
	Comment 8 • 16 years ago

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/efe3c6f76bca
http://hg.mozilla.org/mozilla-central/rev/e45938aee309
See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229736880.1229736940.3072.gz
Linux mozilla-central leak test build on 2008/12/19 17:34:40

/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp: In member function ‘nsresult nsComponentManagerImpl::HashContractID(const char*, PRUint32, nsFactoryEntry*)’:
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:1301: error: ‘PR_GetMonitorEntryCount’ was not declared in this scope

/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp: In member function ‘nsIModuleLoader* nsComponentManagerImpl::LoaderForType(LoaderType)’:
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:2702: warning: comparison between signed and unsigned integer expressions
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp: In member function ‘void nsComponentManagerImpl::LoadDeferredModules(nsTArray<DeferredModule>&)’:
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:3124: warning: comparison between signed and unsigned integer expressions
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp: In member function ‘virtual nsresult nsComponentManagerImpl::AutoUnregisterComponent(PRInt32, nsIFile*)’:
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:3166: warning: comparison between signed and unsigned integer expressions
}

NB: The 3 warnings are probably not yours, but if you could have a look at them too...

	Sergey Yanovich Assignee
	Comment 9 • 16 years ago

* fixed debug build by including "private/pprthred.h"
* fixed signed/unsigned mismatch warnings

Can r+/a+ be carried over in this situation?

	Benjamin Smedberg
	Comment 10 • 16 years ago

Comment on attachment 353965 [details] [diff] [review]
patch v2.1

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

>+/* Linux/Win32 export private NSPR files to include/private */
>+#ifdef XP_MAC

XP_MAC is dead (was MacOS9)

	Sergey Yanovich Assignee
	Comment 11 • 16 years ago

(In reply to comment #10)
> XP_MAC is dead (was MacOS9)

Any comments how it should look like instead? I copied the include block from here:
http://mxr.mozilla.org/mozilla/source/xpcom/base/nsGarbageCollector.c#54

	Sergey Yanovich Assignee
	Comment 12 • 16 years ago

* plainly include "private/pprthred.h", following this:
 http://mxr.mozilla.org/mozilla/source/netwerk/base/src/nsFileStreams.cpp#49

* use special macro PR_InMonitor() instead of PR_GetMonitorEntryCount()

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 13 • 16 years ago

Comment on attachment 354404 [details] [diff] [review]
patch v2.2

Please only ask for 1.9.1-approval once you've gotten review and the patch has landed on mozilla-central.

	Benjamin Smedberg
	Comment 14 • 16 years ago

Comment on attachment 354404 [details] [diff] [review]
patch v2.2

>diff --git a/nsprpub/configure b/nsprpub/configure

Extraneous change?

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

>-    nsAutoMonitor mon(mMon);
>+    NS_ASSERTION(PR_InMonitor(mMon), "called from outside mMon");
> 

Go ahead and make this NS_ABORT_IF_FALSE, here and below.

>@@ -3163,7 +3164,7 @@ nsComponentManagerImpl::AutoUnregisterComponent(PRInt32 /* unused */,
>     nsCOMPtr<nsIModule> module;
>     rv = mNativeModuleLoader.LoadModule(lf, getter_AddRefs(module));
>     if (NS_FAILED(rv)) {
>-        for (LoaderType i = 0; i < mLoaderData.Length(); ++i) {
>+        for (LoaderType i = 0; (PRUint32) i < mLoaderData.Length(); ++i) {

I don't like ridealong changes... please put this in a separate bug/patch. Also, I prefer C++ initializations instead of C-style casts, so

PRUint32(i) < mLoaderData.Length()

The core of this patch looks good. Give me a clean one and I can r+ it right away!

	Sergey Yanovich Assignee
	Comment 15 • 16 years ago

* uses NS_ABORT_IF_FALSE instead of NS_ASSERTION;
* leaves out warning fixes.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 16 years ago

Pushed fc85349c89b4

	Dão Gottwald [:dao]
	Comment 17 • 16 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7d740ae63d5c