Closed Bug 307953 Opened 19 years ago Closed 7 years ago

xpconnect locking is expensive for DOM operations

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: bryner, Unassigned)

References

Details

Attachments

(1 obsolete file) 

I'm trying to evaluate what sort of performance boosts we could get if we
restrict ourselves to a single-threaded Javascript execution.  As a first step,
I'd like to add a configure flag to support building with JS_THREADSAFE
undefined, and fix up callers of threading functions in the code.  I went with
adding macros that are stubbed out for non-JS_THREADSAFE builds, for simplicity
at the call sites.
Attached patch patch (obsolete) — — Splinter Review 
This patch yields a very modest speedup on Tdhtml (1.5-2.0%), but it doesn't
yet condition any locking inside XPConnect on JS_THREADSAFe.  My initial
testing indicates that that could yield a much bigger speedup when manipulating
many DOM nodes, but I'm still working out the details of that part of the
patch.
Attachment #195589 - Flags: superreview?(darin)
Attachment #195589 - Flags: review?(brendan)
JS does thread-safety at nearly zero cost, why can't XPConnect?

We're already a platform in which you can use JS, if not XPConnect, on multiple
threads.  LiveConnect, for instance.

XPConnect is used on other threads too, sometimes it even works.  We are heading
toward more concurrency over time, not less, in our own code -- and the platform
will expose it more, not less.

So it's good to know the current costs, but I think we will have to optimize the
MT case, not reconfigure for ST.

/be
(In reply to comment #2)
> XPConnect is used on other threads too, sometimes it even works.  We are heading
> toward more concurrency over time, not less, in our own code -- and the platform
> will expose it more, not less.

More concurrency in what way?  Pretty mcuh all of the platform API that we
provide is in the form of asynchronous callbacks, so you don't have to spin up
new threads to avoid blocking the UI.
(In reply to comment #2)
> So it's good to know the current costs, but I think we will have to optimize the
> MT case, not reconfigure for ST.

If we had infinite time, I'd agree.  As it is, I'm not convinced that the
ability to execute JS on multiple threads, which we don't utilize today in any
of Mozilla's XUL-based applications, is worth the time investment to solve all
of these threading problems in XPConnect in a performant way.
Again, LiveConnect.  Flash also uses threads and JS, IIRC.

You can't renege on MT JS in Gecko, it's been there for 7+ years.

Real DHTML perf wins are coming, some are already here on the 1.9a trunk (e.g.,
bug 299689), from algorithmic fixes to the DOM and related code.

/be
fwiw, xpinstall uses background threads. extensions might use threads. not
everything can be done in the main thread only.
bleh, whatever.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → INVALID
bryner: don't give up so easily.  What you did in measuring Tdhtml cost of JS
engine-level locking was cool (was that on a Mac?).  We could use more of that
kind of A-B comparison and measurement.  Why not finish the job and see what
XPConnect's cost is?

I hope you reconsider and reopen this bug.

/be
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Oops, I didn't mean to reopen, as my comments implied.

bryner, if you don't want this bug, can you attach whatever other patches you
might have and give it to nobody?  Someone else (maybe me) may take up the charge.

/be
(In reply to comment #8)
> engine-level locking was cool (was that on a Mac?).  We could use more of that

It was on a Mac, yes.  I notice there's no hand-rolled compare-and-swap in
jslock.c for Mac.  I'm not that much of a PowerPC expert, but it seems like we
could do better, based on googling "powerpc compare and swap".

One of the big performance-killers seems to be that getting ahold of a cached
xpconnect wrapper always requires locking (multiple times, actually).  This is
particularly wasteful for DOM nodes, since they are in no way thread-safe.  One
idea I had, that shouldn't impact functionality, is to add to nsIXPCScriptable
to allow nsNodeSH etc. to assert single-threaded use.  Then, those wrappers
could be entered into a separate hashtable, which we would consult first,
without locking.
Status: REOPENED → ASSIGNED
Summary: Support building gecko with JS_THREADSAFE undefined → xpconnect locking is expensive for DOM operations
The jsapi.h patch and its call-site effects is something to consider on its own,
but the bulk of this bug seems targeted at XPConnect.

/be
Component: JavaScript Engine → XPConnect
Argh, I forgot about bug 296879 and its dependency, bug 296878.  We should make
an optimized js_CompareAndSwap for the Mac, for sure!

/be
Depends on: 296879
> As it is, I'm not convinced that the ability to execute JS on multiple threads,
> which we don't utilize today in any of Mozilla's XUL-based applications

It's used in Chatzilla, at least...

That said the XPConnect locking is definitely an issue I've seen popping up in
profiles, and anything we can do to make it better would be great.
> Real DHTML perf wins are coming, some are already here on the 1.9a trunk (e.g.,
> bug 299689), from algorithmic fixes to the DOM and related code.

We're getting close to some fundamental limits here, actually.  Except in
algorithmic-complexity cases like bug 299689, DOM+layout is about 25% of a
typical DHTML profile.  The rest is mostly in security manager, JS, and
XPConnect, from what I can tell.  Given that Opera 8 is 2-5 times faster than we
are on those same testcases, we could clearly do better....
> One idea I had, that shouldn't impact functionality, is to add to
> nsIXPCScriptable to allow nsNodeSH etc. to assert single-threaded use.

That sounds like a really good idea, actually!  Do we still have a free flag there?
can we reuse nsIClassInfo::MAIN_THREAD_ONLY maybe?
Yeah, it looks like the flag is already there, and is just used for debug
threadsafety checks at the moment.

So supposing we did something like this:

if (PR_GetCurrentThread() == NS_UI_THREAD) {
  // check main-thread wrapper cache
}

// lock
  // check multi-thread wrapper cache
// unlock

then in the case where we have to create a wrapper, return an error if
MAIN_THREAD_ONLY is set and we're called from another thread.

... is that going to cause any problems, or could we do better?
> Again, LiveConnect.  Flash also uses threads and JS, IIRC.

bryner and I discussed liveconnect and flash scripting the DOM of the page.  how
exactly could either of those get away with accessing our DOM from a background
thread when our DOM is not threadsafe?  and if they are not touching our DOM,
then what are they touching?  do you have any concrete examples of web content
triggering script execution on a background thread (including use cases
involving plugins)?

biesi: any pointers to the xpinstall code that does that?  (why in the world
would xpinstall need to spawn a thread for JS execution anyways?)
> It's used in Chatzilla, at least...

really?!  can you point me at the chatzilla code that runs on a background thread?
> bryner and I discussed liveconnect and flash scripting the DOM of the page.

Note that not all XPConnect access there has to be accessing the DOM of the
page.. they could be accessing other parts of XPCOM (eg necko).

> can you point me at the chatzilla code that runs on a background thread?

At the moment, the DNS resolver callback.  And yes, I know we have a bug on
that, but they might end up solving it by moving that code out of a window into
a component.
(In reply to comment #18)
> > Again, LiveConnect.  Flash also uses threads and JS, IIRC.
> 
> bryner and I discussed liveconnect and flash scripting the DOM of the page.
> how exactly could either of those get away with accessing our DOM from a
> background thread when our DOM is not threadsafe?

Who said anything about the DOM?

biesi mentioned xpinstall, that's another requirement for JS_THREADSAFE.

> and if they are not touching our DOM,
> then what are they touching?  do you have any concrete examples of web content
> triggering script execution on a background thread (including use cases
> involving plugins)?

JS is not useless without the DOM, you know.  It can be used in conjunction with
Java and Java's class library to do lots of stuff.  Including network and file i/o.

My Flash memory was of a stack we saw, within the last year, showing FlashPlayer
code running on a thread it created, and calling JS.  I'll try to find the bug.

But your line of argument is silly.  If you think we can go back to Netscape 2
and 3, which were single-threaded apart from Java threads, which when connected
(Netscape 3 only) via LiveConnect to JS had to proxy to the "mozilla thread" and
nest event loops to handle nested JS/Java calls -- in short, where "we" (who?)
own all the C and C++ code and can dictate arcane single-threaded control flow
rules and restrictions -- you are welcome to that past life regression!

Threads are the right solution where concurrency that can be mapped onto
hardware parallelism is required.  (Concurrency not requiring h/w parallelism,
in fact constrained to allow coroutining, is also useful, and JS2 is going to
support that kind of concurrency via generators.)

Callbacks with explicit state machines and control blocks suck.  We've had too
many bugs, including security bugs, due to their complexity.

The grass is not necessarily greener with threads, but it can be -- and we are
already using threads other than the main thread for JS in order to avoid the
scaling and maintenance problems of the non-threaded callback/continuation
style, and we will tend to do so more, not less, over time.

Other platforms don't have our main-thread-only restriction.

MS of course supports threads, including via Apartment Threading in COM, and as
a primitive in the runtime in .NET.

Opera flattens control flow into single-threaded, non-reentrant continuation
passing, AFAICT, which is why it can't yet implement the new NPRuntime extension
to the plugin API that Safari and Firefox support.

This points out a practical downside of "anti-threading."  If the NPRuntime
extension had followed Opera's execution model, every plugin vendor would have
to split plugin code reached from a script up into a maze of twisty callbacks. 
No control flow could reenter JS from the plugin called from JS, or vice versa.
I know this is now what you are explicitly proposing, but this kind of mess will
follow from turning off JS_THREADSAFE and retrenching to a single-threaded C and
C++ platform.  Assuming we can unbreak LiveConnect, Flash, xpinstall, ....

If JS is < 2% (a lot less on non-Mac tier 1 platforms), why are we even wasting
time on it?  The high order problems are either algorithmic (XPConnect locking
when it should not, document.getElementById suffering O(n^2) growth) or else
architectural (hard to fix without big changes).

/be
(note: I'm not 100% certain that my code references to xpinstall are right.
however, I do know for certain that it uses threads, if for nothing else because
I occasionally see threadsafety assertions from it.)
Yes, the xpinstall bits that process "isntall.js" do so on a background thread.

I don't think that removing threadsafe JS (non-DOM) is a good idea: I know
several private "mozilla as platform" projects that made extensive (and correct)
use of threads from JS components.

I *do* think that reusing the classinfo mainthread/singlethreaded flags could be
a huge win (and assertions when people tried to use DOM objects in threaded code
would be really helpful).
> And as far as chatzilla goes, see
> https://bugzilla.mozilla.org/show_bug.cgi?id=306482#c16

Yeah, that's a bug.  In other words, chatzilla isn't supposed to be using
multiple threads.
Why not?  Using multiple threads is not a bug.  Using multiple threads off a DOM
window is a bug.  They're planning to move the code to an XPCOM component, where
it can happily use multiple threads.  That's what that comment said, as I
understood it.
> Who said anything about the DOM?

I believe that DOM access from web pages accounts for the majority of JS use
cases on the web, so I think it is pertinent to this discussion.


> biesi mentioned xpinstall, that's another requirement for JS_THREADSAFE.

Fair enough.  I can appreciate that it simplifies install.js processing to do
things as they are requested instead of deferring them until later or something
like that.


> JS is not useless without the DOM, you know.  It can be used in conjunction 
> with Java and Java's class library to do lots of stuff.  Including network and 
> file i/o.

Sure.  I realize that JS can invoke Java APIs, but in relative terms this use
case is hardly seen on the web.  I'm interested in optimizing for the common JS
use case -- namely DOM access.


> But your line of argument is silly.  If you think we can go back to Netscape 2
> and 3, which were single-threaded apart from Java threads, which when 
> connected (Netscape 3 only) via LiveConnect to JS had to proxy to the "mozilla 
> thread" and nest event loops to handle nested JS/Java calls -- in short, where 
> "we" (who?) own all the C and C++ code and can dictate arcane single-threaded
> control flow
> rules and restrictions -- you are welcome to that past life regression!

Maybe I am extremely naive, but I don't believe that users encounter many pages
that utilize the ability to execute JS on a background thread.  Am I mistaken?
(Anyways, I thought that the whole business of proxying JS calls had to do with
Java executing on a separate thread in those older browsers.  I didn't think
that architecture persisted with the OJI-based plugin.)


> Threads are the right solution where concurrency that can be mapped onto
> hardware parallelism is required.  (Concurrency not requiring h/w parallelism,
> in fact constrained to allow coroutining, is also useful, and JS2 is going to
> support that kind of concurrency via generators.)

Sure, but I don't see us making use of multiple threads today.  Are you saying
that you see that changing in the near term?  Are there plans a-foot to
parallelize portions of our rendering engine (or other expensive and costly
operations performed by our platform)?


> The grass is not necessarily greener with threads, but it can be -- and we are
> already using threads other than the main thread for JS in order to avoid the
> scaling and maintenance problems of the non-threaded callback/continuation
> style, and we will tend to do so more, not less, over time.

You're referring to XPinstall here, or are there other use cases in the browser
today (excluding plugins)?

I think that good non-threaded callback/continuation style APIs make it easier
for people to write code quickly.  Thread synchronization is a major headache
and is, I believe, more costly in terms of development time.  If you can stay on
a single thread, then save yourself the headache, and do it.  In my experience,
the really painful problems stem from unexpected re-entrancy.  This can occur
whether you are using threads or callbacks, but I believe that I've seen far
worse cases with threads (in our code at least).


> Other platforms don't have our main-thread-only restriction.

Sure, many other platforms do (e.g., Xwindows -- bad example, granted).


> I know this is now what you are explicitly proposing, but this kind of mess 
> will follow from turning off JS_THREADSAFE and retrenching to a 
> single-threaded C and C++ platform.  Assuming we can unbreak LiveConnect, 
> Flash, xpinstall, ....

Yeah, this kind of example is very compelling.


> If JS is < 2% (a lot less on non-Mac tier 1 platforms), why are we even 
> wasting time on it?  The high order problems are either algorithmic (XPConnect 
> locking when it should not, document.getElementById suffering O(n^2) growth) 
> or else architectural (hard to fix without big changes).

This bug is about xpconnect ;-)  What I'm interested in is improved DOM
performance.  The other major browsers have a much faster DOM implementation
(more than twice as fast in many cases).  It seems like xpconnect is a large
part of the cost here, so the idea is to eliminate some of that cost.

You've convinced me that we cannot disable threading support in JS.  At least,
we have one consumer in the tree (XPInstall) and probably some web content out
there that would be broken by the change.  The NPRuntime issues also sell the
point.  OK.

So, let's see what we can do to improve xpconnect.
> Why not?  Using multiple threads is not a bug.

Sure, in general multiple threads is not a bug.  I wasn't arguing that it was. 
I was pointing out that when they fix that bug, chatzilla will not be using
multiple threads.  If they intend to move the code to a new architecture that
does use multiple threads, then that's news to me.
Untrusted content does not do multiple threads. The xpconnect locking debate is
about xpcom components written in JS (because that's the only safe way to write
multithread JS).
(In reply to comment #17)
> So supposing we did something like this:
> 
> if (PR_GetCurrentThread() == NS_UI_THREAD) {
>   // check main-thread wrapper cache
> }
> 
> // lock
>   // check multi-thread wrapper cache
> // unlock
> 

I coded up this approach and I don't see a measureable win on my testcase (which
is just to create a whole bunch of elements).  I think part of the reason is
that it still locks to access the multi-thread wrapper cache in the case where
the wrapper doesn't exist yet.  I'd like to avoid that, but the only way I can
see to do so is to move up the call to GatherScriptableCreateInfo so that we can
check for the MAIN_THREAD_ONLY flag.  I think that might slow things down too much.

So, another idea would be to make the locking less expensive.  Bug 167544 might
be one answer to this... Brendan, do you know of any code in Spidermonkey that
might be useful here?

> then that's news to me

This is why I pointed to comment 16, not to the bug in general.  To quote the
relevant part of that comment:

  Note: all of the core code *will* be ending up within the scope of a JS XPCOM
  Component prior to CZ 1.0, so this will revert back to proper threading at
  that point.

So in any case, it sounds like we agree that we can't just disable threading and
we agree that the locking overhead in XPConnect is biting us and that hence
anything we can do to reduce that overhead while retaining multi-thread stuff is
a good thing.  Does that sort of sum up things?
> move up the call to GatherScriptableCreateInfo so that we can
> check for the MAIN_THREAD_ONLY flag.  I think that might slow things down too
> much.

So... if we're optimizing for the single-thread case, can't we do this:

  Check main thread map.
  If not found, gather scriptable info.
  If not main thread only, check multithread map.

After all, for the common case (DOM nodes) we want to gather the scriptable info
anyway, no?
Yeah, I was just sort of concerned that this would be too much overhead for
accessing non-DOM stuff.  I'll do some more testing though.

(In reply to comment #33)
>   Check main thread map.
>   If not found, gather scriptable info.
>   If not main thread only, check multithread map.


(In reply to comment #28)
> > Who said anything about the DOM?
> 
> I believe that DOM access from web pages accounts for the majority of JS use
> cases on the web, so I think it is pertinent to this discussion.

Yeah, but it's not exclusive of other uses that do need thread safety, which
we've already been over.  Components, plugins, and such use JS on other threads
than the main thread.

What's going on here, rhetorically?  Above, and later below in your comment to
which I am replying, you repeat "B!" in response to my "A", but B does not
contradict A, and I never, ever, not even once said "not B!".  Is it that you
want "B" (most of our platform and extension code is single-threaded) to be
given more prominence even as we address point A in this bug?

In the interest of cutting down noise on this bug, I'm not going to write a
point by point reply.

I will say (again) that there's no plan that I know of to use threads for core
content, layout, or rendering code.  That's irrelevant to our exchange.  It's
enough that Necko uses threads to see why other platform pieces and customers
may want to use threads, in the future.  That "platform peer equity" plus the
tendency of JS to creep into any control flow graph means we need thread safety
in JS and XPConnect for the future, ignoring existing compatibility constraints.

It seems (correct me if I am wrong) as though everyone cc'd here agrees that (1)
JS_THREADSAFE is here to stay; (2) XPConnect needs optimizing to avoid stupid MT
overhead for main-thread-only JS/C++ XPConnections.  I don't see  (2) costing us
a lot of effort in absolute terms (not merely relative to turning off low-level
MT support).

It's great to get some movement on the PowerPC atomic instruction patch, too.

/be
(In reply to comment #34)
> Yeah, I was just sort of concerned that this would be too much overhead for
> accessing non-DOM stuff.  I'll do some more testing though.
> 
> (In reply to comment #33)
> >   Check main thread map.
> >   If not found, gather scriptable info.
> >   If not main thread only, check multithread map.

I bet it won't be too much overhead, but if it is, we could optimize locking for
these maps.  I'll look at bug 167544; we should make it block this bug if that's
appropriate.

The compare-and-swap optimized locking that JS does is actually done by many
modern OS libraries above the system call layer, so I'm not sure we need to
spread it around.  It's in JS because JS is old -- it was done in '98 and '99.

The other big optimization in JS is peculiar to JS's "request model", which uses
the GC's barrier to amortize object locking costs almost to zero.  That could
help XPConnect, but first we would have to bite the bullet and fix bug 176182 --
and I'm not in a hurry to do that (darin will be relieved to know!).

As bryner and darin note, we do indeed have an asymmetric thread model in our
own code and much of our platform, and lack of requests on the main thread is a
win in terms of simplicity and lower call costs.  It's also a major assumption
that's deeply buried and intertwined with DOM and other code.

/be
(In reply to comment #36)
> The compare-and-swap optimized locking that JS does is actually done by many
> modern OS libraries above the system call layer, so I'm not sure we need to
> spread it around.  It's in JS because JS is old -- it was done in '98 and '99.

If someone has data showing that JS when compiled with JS_USE_ONLY_NSPR_LOCKS
performs significantly worse than the way JS works by default on platforms where
it uses the compare-and-swap thin locks, please let me know.

I wrote the above-cited words about '98 and '99 to give the main reason this
code is there (chronology, precedence), but it is not obvious that we should
turn it off and save ourselves the trouble of fixing bug 296879.  It may be that
modern OSes don't do as good a job on all the platforms we care about, or that
the layering is still thick enough that we win with the thin-lock code.

/be
Brendan:  I agree that 2 seems like the best path to follow.

As for existing use cases of threads in our code, I think that most of those are
to work around limitations of OS APIs (getaddrinfo/gethostbyname, poll/select,
file read/write, etc.).  If the OS provided good, asynchronous APIs, then we
wouldn't need to develop our own.  So, in the vast majority of cases, we only
use threads because there is no other way.  That said, there are notable
exceptions.  XPinstall and the HTTP connection manager both benefit from being
able to run independent of the main thread.

Ugh, I'm not sure I know exactly what you mean by A and B.  That's okay, I think
we've beaten this horse to death ;-)
(In reply to comment #38)
> Ugh, I'm not sure I know exactly what you mean by A and B.  That's okay, I think
> we've beaten this horse to death ;-)

One more kick:

A.  We should fix XPConnect locking to not suck, especially for main-thread-only
objects and control flows.

B.  Most of our code and platform is main-thread-only, we should optimize for
this case and not bend over backward for exceptions to it.

I think B is your point, but the implications of not bending over backwards are
unclear.  It's clear it doesn't mean "turn off thread safety".

Thread-safety in a single-process, shared-source codebase with no kernel-like
(hardware MMU protected) interface tends to be all-or-nothing.  "Platform-ness"
tends to spread, often where you least want or expect it (because *our* internal
APIs are handy, or *we* find threads convenient, who would have thought others
building alongside or above the platform would too?).

I continue to believe that we will see more threading in the future, outside of
the main layout control flow, but I'm not sure what it will be used for.

We've all been laughing at those "thread-safe nsISupports" assertions warren
added years ago, but who will have the last laugh?  Probably someone else who
builds on the platform, uses threads for good design reasons, and creates
something so popular that we have to fix more of the thread-related bugs hiding
in our codebase in a firedrill, just to support that popular thing.

/be

Comment on attachment 195589 [details] [diff] [review]
patch

Reviewing, since this patch doesn't say that we will be disabling
JS locking.  It just makes us honor MOZ_JS_THREADSAFE properly.


>Index: configure.in

Since we're never going to use it, the configure option is probably
something you could leave out.	But, technically it looks fine to me,
and I don't have a preference one way or the other about landing it.



>Index: xpinstall/src/nsSoftwareUpdateRun.cpp

You might want to go one step further here, and add a #error if
someone tries to build this code without threadsafe JS.


r/sr=darin
Attachment #195589 - Flags: superreview?(darin) → superreview+
Depends on: 314285
Attachment #195589 - Attachment is obsolete: true
Attachment #195589 - Flags: review?(brendan)
I'm not actively working on this one.
Assignee: bryner → dbradley
Status: ASSIGNED → NEW
QA Contact: general → pschwartau
(In reply to comment #41)
> I'm not actively working on this one.

Did you or feng have newer profiling data that sheds light on the severity of this bug?  Or is the old profiling data suspect?  Maybe oprofile is necessary.

/be
Assignee: dbradley → nobody
(In reply to comment #42)
> (In reply to comment #41)
> > I'm not actively working on this one.
> 
> Did you or feng have newer profiling data that sheds light on the severity of
> this bug?  Or is the old profiling data suspect?  Maybe oprofile is necessary.
> 
> /be
> 

In my learning process, I did some profiling and hacking on XPConnect map locks. I compared two configurations: one assumes single-thread model, so XPCAutoLock constructor does nothing, and another one uses PR lock as in the current code. It is a bit harder to find DOM-maniputation intensive benchmarks. I used the table sorting benchmark (from http://wwwfx.eae.net/dhtml/tablesort/tablesort.js), gmail, google map, page load, and calendarhub.com. Only the first benchmark has a lot of XPConnect activities.

What I profiled is the cost of locking to access maps in XPConnect (bug 167544). On Linux, table sorting got about 5% speedups by disabling these locks. That's the limit that we can get out.

What's interesting is that I dumped the runtime characteristic of each map, and it turns out that majority of locks happened on two map types: IID2NativeInterfaceMap and Native2WrappedNativeMap, and most of these locks protect map lookups.

As mentioned in bug 167544, thin lock would reduce the lock overhead. But I tried another approach that creates thread-local IID2NativeInterfaceMaps, so no locks are needed to access IID2NativeInterfaceMaps. I will attach the patch to bug 167544 to get some discussion and feedbacks.
QA Contact: pschwartau → xpconnect
XPConnect is single threaded now.
Status: NEW → RESOLVED
Closed: 19 years ago → 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: