120506 - Monitor entered twice causing performance hit

Status: RESOLVED WORKSFORME

(Core :: XPConnect, defect, P3)

Description

[Daniel Bratell]

When converting between js and "native" (which is an expensive operation
according to profiling) the thread safety overhead is the largest offender.

One easy target, I think, is that XPCNativeInterface::GetNewOrUsed and
XPCWrappedNative::GetNewOrUsed are called together, both calling
PR_Enter_Monitor and PR_Exit_Monitor. So the code enters and leaves the monitor
twice.

The PR_Monitor calls are 3-4% of the total time spent in the getElementById
testcase in bug 118933. The improvements made to CanAccess and ResolveName colud
bring this up to ~10%.

Comment 1

[Daniel Bratell]

One soulution could be to create _unlocked versions of the functions, leaving
the PR_Monitor calls to the caller, in this case
XPCConvert::NativeInterface2JSObject. 

Comment 2

[David Bradley]

That would only eliminate two or three of the calls per invocation of
XPCNativeInterface::GetNewOrUsed. There are many other functions that get called
from there that also lock the maps and often these functions end up in a
limitted recursion. The biggest offender in this case is getting the thread
local storage data, since most of these calls don't even lock because if's
already locked.

The locks end up at EnterCritical section for the Windows platform. Thus what
the monitor is doing is somewhat unecessary on the Windows platform. Is this a
unique thing to the Windows platform or do we have a cross platform
implementation that is pesimistic and with some additional platform tweaks we
could see some nice improvements not just for this case but anything that uses
the monitor.?

Comment 3

[David Bradley]

I'm going to future this for now, rather than close it. It may be worth taking a
look at later.

Comment 4

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

Could this bug reassigned to NOBODY?

Comment 5

[David Bradley]

If someone else wants to pick this up feel free. I'll be happy to help out.

Comment 6

[Wayne Mery (:wsmwk)]

timeless,  what profiling is needed to determine the hit of switching btwn js and "native" in thunderbird, and whether the changes suggested here might usefully benefit thunderbird?

Comment 7

[timeless]

dunno. i think any normal time (Txul, Tp, Ts,...) test would be good enough (or use the testcase mentioned in comment 0).

i think daniel if he's still around could write the patch, or anyone else could based on his suggestion.

i guess one question is trying to find the best set of functions to combine w/in a single entered monitor, or a set of data bits that can be retrieved once and passed around instead of needing to grab the monitor and check the bits again.

Comment 8

[David Bruant]

Still a valid bug or moving lots of things to WebIDL fixed it?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Nuking threadsafety from xpconnect fixed this bug.  Not using xpconnect at all is just gravy.  ;)