46703 - bind DOM standard classes lazily

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Warren Harris]

From the footprint meeting 7/26/00:

2) "Super global" for JavaScript
  Description: Currently each DocShell has a script context and script global 
object associated with it. For each script global 
  object, we initialize the JavaScript core classes and some DOM classes per 
document in the shell. This redundancy has both 
  performance and bloat repercussions. The thought is that we might be able to 
have "super global" object on which the core 
  classes are defined. This would be the JS prototype of the per shell script 
global.
  Module owner: [e-mail]
  Task owner: [e-mail], [e-mail]
  Status: JBand to determine the memory used by the core classes. JBand, Vidur 
and Johnny to determine what could break 
  with a super global.

Comment 1

[David Hyatt]

XBL also does per-document initing of its JS classes.  This would help its 
performance as well.

Comment 2

[John Bandhauer]

I did a quick intrumentation of JS_malloc et al. I turn it on at
the start of nsJSContext::InitContext and off at the end. It
gives me the following on a document reaload:

        g_DEBUG_MemAllocCount = 2700
        g_DEBUG_MemRellocCount = 214
        g_DEBUG_MemFreeCount = 28
        g_DEBUG_MemTotalAllocBytes = 86489

That last number is only the total of the bytes for which
JS_malloc was called. It does not take into account the realloc
calls or (especially) the 28 free calls. So 86k is more-or-less
the upper boundray of savings per window when window count is >
1. I might write some code with a hashtable to track all the
blocks and give a better total number. There are also malloc
calls in JS that bypass JS_malloc. An incremental bloat blame
would do better to give us real numbers.

It is likely that some of the free calls are for big blocks;
i.e. arenas.

I'm pretty afraid of the complications of trying to do a super
glabal scheme that doesn't break anything. This is looking like a
smallish win to me right now.

Comment 3

[Warren Harris]

86k per window is small?

Comment 4

[John Bandhauer]

Something less that 86K - maybe a lot less. I was suggesting that it was not 
worth chasing right away. When we have bloatblame support for snapshots we can 
get a better number and decide to attack here or not.

Comment 5

[John Bandhauer]

more data...

I added a quicky hashtable to track allocated sizes so that I could track sizes 
of free'd blocks (including reallocs). g_DEBUG_MemTotalAllocBytes now includes 
reallocs too. g_DEBUG_MemTotalFreedBytes includes frees and abandoned realloc'd 
blocks.

I also threw in a GC before starting and ending to whack any hangers-on 

The net memory usage is still about 86K 

g_DEBUG_MemAllocCount = 2701
g_DEBUG_MemRellocCount = 221
g_DEBUG_MemFreeCount = 33
g_DEBUG_MemTotalAllocBytes = 99713
g_DEBUG_MemTotalFreedBytes = 12750
net alloc'd bytes = 86963

There is a real potential gain here. I still fear the cost.

Comment 6

[Warren Harris]

You can do it!!!

Comment 7

[John Bandhauer]

warren's confidence is inspiring.

cc'ing brendan and shaver in case they have insights to offer.

Comment 8

[Akkana Peck]

Please make sure when you're done that JS is still turned off on edited pages
(or let me know when to check, and I'll help test), since the editor needs to be
able to disable JS on a per-document basis. Thanks!

Comment 9

[John Bandhauer]

I realized that some space used was DEBUG only. A release build gives numbers 
like:
g_DEBUG_MemAllocCount = 2458
g_DEBUG_MemRellocCount = 202
g_DEBUG_MemFreeCount = 29
g_DEBUG_MemTotalAllocBytes = 89992
g_DEBUG_MemTotalFreedBytes = 11640
net alloc'd bytes = 78352

I traced through this stuff and there are no drastic mallocs going on anywhere 
or anything. This is all from the expected activities of defining functions and 
properties etc. There are just a lot of them.

About 33K is attributable to the JS standard classes. The rest is distributed 
among the classes intited in nsJSContext::InitClasses.

If the super-global scheme is going to break too much stuff then there are still 
some posibilities...

The standard classes could share function structs (i.e. clone function objects) 
There is a small savings there. Though it adds complexity.

We could get rid of some of the inits which add named constructors and have them 
use the NameSpaceManager instead to be lazily init'd.

vidur says that some of these things don't require init at all.

Some of the standard classes are big - e.g. Date. We could extend the JS API to 
allow them to participate in the NameSpaceManager thing and have them be lazily 
inited.

Brendan, I'm interested in your thoughts. I'm thinking that the super-global 
solution is just going to break too much stuff and allow for unexpected data 
leakage.

Comment 10

[Warren Harris]

I'd really like to see us bite the bullet on this one and just do it. 78K is 
simply too much per docshell instantiated.

Comment 11

[Brendan Eich [:brendan]]

78K is bigger than I'd like, and we can do some things (more on that below), but
before anyone declares this priority #1, can someone show me the remaining per
DocShell costs?

I think we should use lazy techniques where possible for all the standard core
JS and DOM classes, and I'd be happy to help extend the JS API accordingly, if
it makes sense.  One way to do it that requires no API extension, however, is to
"fault in" cloned function objects for all the standard constructors, only when
referenced, from a superglobal prototype.  The Math object requires special
handling, as it is not a function object (it's a prototype of class math_class).
But it has no private data.  Here's a general clone-on-demand sketch, assuming
JSVAL_IS_STRING(id) for an id passed to resolve:

    JSString *str = JSVAL_TO_STRING(id);
    jschar *name = JS_GetStringChars(str);
    size_t length = JS_GetStringLength(str);
    uintN attrs;
    JSBool found;

    if (!JS_GetUCPropertyAttributes(cx, superglobal, name, length,
                                    &attrs, &found)) {
	return JS_FALSE;
    }
    if (!found)
	return JS_TRUE;

    jsval v;
    if (!JS_GetProperty(cx, superglobal, name, length, &v))
	return JS_FALSE;
    JSType type = JS_TypeOfValue(cx, v);
    JSObject *obj;
    if (type == JSTYPE_FUNCTION) {
	obj = JS_CloneFunctionObject(cx, JSVAL_TO_OBJECT(v), global);
	if (!obj)
	    return JS_FALSE;
	v = OBJECT_TO_JSVAL(obj);
    } else if (type == JSTYPE_OBJECT && (obj = JSVAL_TO_OBJECT(v)) != NULL) {
        obj = JS_NewObject(cx, JS_GetClass(cx, obj), obj, global);
	if (!obj)
	    return JS_FALSE;
	v = OBJECT_TO_JSVAL(obj);
    }
    return JS_DefineUCProperty(cx, global, name, length, v, NULL, NULL, attrs);

Something like that should work for the standard classes.  Let me know.

If we do this, we should fix LiveConnect to be lazily initialized too.

/be

Comment 12

[Brendan Eich [:brendan]]

BTW, I'm fixing bloatblame bugs and working on bloatstrip, but you can do
"bloatblame snapshots" now, using TraceMallocDisable and Enable.  But forget
snapshots: bloatblame from startup to first window shows DocShell costs that
dwarf the JS ones.

I agree we should make things space-efficient if possible, but we are short on
time, and we may have bigger wins elsewhere.  I'm open to this superglobal
scheme if it can be done compatibly, but where are the numbers by which we are
prioritizing?

/be

Comment 13

[Warren Harris]

I think this should be nsbeta3+ -- jband let me know if you disagree...

Comment 14

[Brendan Eich [:brendan]]

I believe my sketch will work (with typo fixes such as JS_GetUCProperty rather
than JS_GetProperty).  However, it introduces security holes and compatibility
bugs: two pages cannot add distinct String.prototype.supersub (e.g.) methods,
they will collide -- which implies security holes: evil.org can wrap commonly
called methods and spy on victim.com's string data (e.g. again).

Fixing these holes will be tougher.  Is it worth the effort?  Warren, show me
the numbers that make this more important to fix than other bloat bugs.  I'm
sympathetic on general principles, if we had infinite time to fix all bloat
bugs.  We don't.

/be

Comment 15

[John Bandhauer]

I agree with brendan. I think there are bigger fish to fry. By coming up with a 
much better strategy than I had in mind, brendan has already demostrated to me 
that I'm not going to succeed on this without a bunch of help from him anyway. 
I don't have a clue on how to deal with the holes this uncovers. We should hold 
this one in reserve and focus on bigger problems.

Comment 16

[Warren Harris]

Come on guys... we're talking 78k per docshell!!! Show me a bigger fish and I'll 
fry it.

Comment 17

[Brendan Eich [:brendan]]

Ok, another try, along the lines jband suggested (API change): add a new JS API
entry point, JS_ResolveStandardClass(JSContext *cx, JSObject *obj, jsval id),
that will initialize a JS class in obj iff a standard class is named by the name
in string-valued id.  Have nsJSUtils::nsGenericResolve try this as a last ditch.
Comments?

/be

Comment 18

[John Bandhauer]

Brendan,

That seems pretty good. Though... those props won't be enumerable unless 
accessed, right? Also, I'm concerned that this will get called too much as 
global properties are resolved. Maybe it is not so bad.

I was talking with mccabe and he imagines something more transparent. But we 
couldn't come up with a mechanism. I thought of two schemes on the way home... 
One involves the registration of tinyids and additional cases in the switch 
statement of the global object. The other idea is simpler and more transparent 
for the caller...

We define an engine internal JSClass as a placeholder for the standard objects. 
Its private data is just an identifier saying which type it is in place of. 
JS_InitStandardObjectsLazily would attach these placeholders as named props of 
the global object. In js_GetProperty (or js_LookupProperty???) we add code to 
always check if the jsval in the slot is a JSObject && of placeholder class. If 
so, we stick the real object in the slot and return it to the caller. We also 
deal with any dependent objects here too.

In theory this seems simple. I'm concerned that the overhead might be a bit much 
(though it is really just a pretty quick test, but on all acceesses). I'm 
also concerned that it might not be so easy to localize such a change in the 
engine. There are various places where the slots are directly accessed. We have 
to be certain that the placeholders never leak out or actaully get accessed 
internally other than when we are replacing them.

What do you think? Your suggestion is simpler.

Comment 19

[Brendan Eich [:brendan]]

We have an enumerate hook, the DOM should use it to reflect eagerly those
properties that are otherwise lazily reflected (only on property access, not on
for..in enumeration).

I don't think we should add more random logic to the engine.  We have sufficient
abstraction to be lazy, we just aren't using it.  The engine does need to help,
in order to avoid spreading knowledge of all top-level property names induced by
standard classes (e.g., parseInt from the Number class).

I may have a patch shortly.

/be

Comment 20

[Brendan Eich [:brendan]]

Changing subject to match reality.

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: proposed lazy-init fix for JS standard classes

Comment 22

[Brendan Eich [:brendan]]

I beefed of jsdhash.[ch] a bit, providing a complete set of stub ops for those
hashtables that store only a const void *key and no value.  I also cleaned up
some (but not all) unshared string constants in the engine.  One constructor,
Date, had to be smartened up to handle a data dependency on cx->runtime->jsNaN
(by calling js_InitNumberClass if jsNaN is uninitialized).

Other than that, the patch is mainly in jsapi.[ch], where the new entry points
are JS_ResolveStandardClass and JS_EnumerateStandardClasses.

/be

Comment 23

[Brendan Eich [:brendan]]

Attachment: slightly better patch (shaver-comment improvement, shared string constants)

Comment 24

[Brendan Eich [:brendan]]

Looking for help testing my patch, and r= love.

/be

Comment 25

[Brendan Eich [:brendan]]

Attachment: oops, left out jsobj.h -- also spruced up a comment

Comment 26

[John Bandhauer]

Brendan,

A few things...

- I don't see locking. Resolve has to be called with the object unlocked. 
Shouldn't you lock in here somewhere? cx->resolving is re-entrance protection 
for the given id on the given JSContext. But what if our object is accessed on 
multiple JSContexts at once?

- The tag scheme is cool AND scary. I guess there is almost zero chance that the 
atom table will live at a *really* low address? :)

- I can't really verify that the name tables have all the right entries. Phil's 
test will help here I guess.

- You're going to fix the problem with the infinite loop we saw with double 
JS_InitStandardClasses calls (and not just fix the call site), right?

I was looking at jsdhash and it bothered me that JSDHashTable is not opaque. I 
understand the reasoning of not *requiring* that it be alloc'd and owned by 
jsdhash. But if you get users outside of the module where it is implemented then 
there is no mechanism to negotiate an agreement or do verification about the 
struct's size. This is bad if the size changes in the future or - more likely - 
jsdhash is compiled with JS_DHASHMETER and the caller is not. Passing 
sizeof(JSDHashTable) somewhere would catch this and fail at runtime rather than 
chance mysterious memory corruption.

I haven't really tested this all much yet. I'm expecting you'll have a new patch 
after you figure out the problems with the tests Phil ran.

Comment 27

[Brendan Eich [:brendan]]

Attachment: restore old global-proto'd-by-Object.prototype logic to avoid infinite loop on double JS_InitStandardClasses

Comment 28

[Brendan Eich [:brendan]]

> I don't see locking. Resolve has to be called with the object unlocked. 
> Shouldn't you lock in here somewhere?

A lot happens under the init function (e.g. js_NewObject for constructor and
prototype objects, which calls down into js_AllocGCThing).  Without well-ordered
locking protocols for the several kinds of locks acquired and released under
init, and including the global object (obj), it seems to me holding the global
object locked would lead to deadlock.

I could add yet another per-runtime lock (a la setSlotLock) and single-thread
all threads in the runtime through JS_ResolveStandardClass, but it doesn't seem
worth it (see below).

> cx->resolving is re-entrance protection 
> for the given id on the given JSContext.

Recursion protection, yes.  Without it, the lookup done by FindConstructor under
each init function's call to js_GetClassPrototype (called from js_NewObject)
will indeed recursively diverge through JS_ResolveStandardClass, overflowing the
stack.

> But what if our object is accessed on
> multiple JSContexts at once?

Top-level objects are generally used by one context at a time anyway, to avoid
name collisions over global variables and race conditions in scripted accesses
and updates.  I think that makes it safe to use JS_ResolveStandardClass from all
global_resolve class hooks.  One would not use it in a shared object, but that's
just a "don't do that" caveat for API consumers (one of many ;-).

But let's suppose two threads managed to race for the first use of "String",
e.g.  The worst that would happen is that duplicate string functions and the
String constructor/prototype would be defined.  The race loser would bind names
to objects that become garbage as soon as the winner rebinds those names to its
objects.  Neither script referencing "String" could run until the winner had
defined the final set of objects for string functions and the String
constructor, so no duplicates would survive and "escape".

> - The tag scheme is cool AND scary. I guess there is almost zero chance that
> the atom table will live at a *really* low address? :)

Not on any extant architecture!  There are some exotic ones that map 0 in
pointer contexts to some strange address, but even those don't allocate at
nearly zero addresses.

> - I can't really verify that the name tables have all the right entries.
> Phil's test will help here I guess.

I made those tables by canvassing all the standard class init functions.

> - You're going to fix the problem with the infinite loop we saw with double 
> JS_InitStandardClasses calls (and not just fix the call site), right?

Fixed in the last patch.  I shouldn't have messed with the logic that I
"rotated" down from JS_InitStandardClasses into js_InitObjectClass, the code
that checks whether the global object has a null proto, and if so, sets that
slot to Object.prototype.

However, we should file a bug, or use this one, on the DOM's (or the docshell's)
crazy double-JS_InitStandardClasses call that my bug disclosed.

> I was looking at jsdhash and it bothered me that JSDHashTable is not opaque.

This bothered wtc too, when I was pitching it at him for NSPR.  He suggested
some alternatives, including adding reserved words at the end of the struct, or
adding a version number at the front.  I like down-and-dirty C data structures,
and am willing to freeze them (see PLHashTable, unchanged in five years since it
began life as PRHashTable).  But you make an interesting suggestion:

> I understand the reasoning of not *requiring* that it be alloc'd and owned by 
> jsdhash. But if you get users outside of the module where it is implemented 
> then there is no mechanism to negotiate an agreement or do verification about
> the struct's size. This is bad if the size changes in the future or - more
> likely - jsdhash is compiled with JS_DHASHMETER and the caller is not. Passing
> sizeof(JSDHashTable) somewhere would catch this and fail at runtime rather
> than chance mysterious memory corruption.

Good idea -- the JS_DHASHMETER stuff is only for power-hackers, but even they
need sanity checks.  However, I'll wait to make this change till after users of
jsdhash.h (bienvenu at least) get a chance to cope.

On to phil's testsuite results.  Another patch coming up.

/be

Comment 29

[Brendan Eich [:brendan]]

Have to move rt->jsNaN/jsNegativeInfinity/jsPositiveInfinity/emptyString init
out of js_Init{Number,String}Class and do it on first context creation.  The
lazy to eager logic was not handling these secret internal variables in general,
only for js_InitDateClass's jsNaN dependency.

Cc'ing mccabe and rogerl, I'll need their code buddying on the next patch.

/be

Comment 30

[Brendan Eich [:brendan]]

Attachment: final patch for the JS engine (I hope!)

Comment 31

[Brendan Eich [:brendan]]

Two fixes:

The names delegated from the global object to Object.prototype, such as
toSource, valueOf, etc., need to be handled specially: a resolve on one of these
names against the global object should call js_InitObjectClass if and only if
the global object has no proto (which means that js_InitObjectClass hasn't been
called yet on that object). 

The second fix, which I had coded and then optimistically thrown away (foolish
me), is to yoke js_InitFunctionClass and js_InitObjectClass as they are in
JS_InitStandardClasses: first you make Function (a function object whose
prototype property is an object with a null proto), then you make Object (a
function object with Function.prototype as its proto, and with a prototype
property whose value is the proto-less object everyone delegates to), then you
set Function.prototype.__proto__ = Object.prototype.  This has to be done when
resolving (the lazy case) as when being eager, because if you just do the
obvious thing (resolve Function by calling js_InitFunctionClass, resolve Object
by calling js_InitObjectClass), you end up with Object being a proto-less
function object, even though Object.prototype is created.

(The recursion through JS_ResolveStandardClass stacks "Function" first, then
"Object", but making the Object constructor fails to find Function bound because
we haven't unwound yet.  So Object is just a proto-less object, which is what
the testsuite was complaining about in 11.2.2-2.js.)

Anyone follow that?

/be

Comment 32

[Brendan Eich [:brendan]]

Attachment: getting there -- only 1 testsuite error with this patch

Comment 33

[Brendan Eich [:brendan]]

InitFunctionAndObjectClasses needed to ensure that both "Function" and "Object"
were in cx->resolving while it ran -- it does that now.

The remaining testsuite failure is going to require some heavy-duty gdb'ing. 
More in a bit.

/be

Comment 34

[Brendan Eich [:brendan]]

Attachment: yay, passes the js/tests suite; good to go?

Comment 35

[Brendan Eich [:brendan]]

The final step was to move the default setting of cx->globalObject into
InitFunctionAndObjectClasses, from just before the call to that function in
JS_InitStandardClasses, so that both the eager (JS_InitStandardClasses) and lazy
(JS_ResolveStandardClass/JS_EnumerateStandardClasses) paths guarantee that
cx->globalObject is set.

Why is cx->globalObject important?  Only if cx->fp is null or cx->fp->scopeChain
is null.  That can occur when calling into the JS API from native code on a cx
that has no stack frames active.

But it turns out that it also arises when you call a heavyweight function (one
requiring a call or activation object) and the Call class has not yet been
initialized (and you're using lazy class init via JS_ResolveStandardClass and
JS_EnumerateStandardClasses).  Code in js_Invoke pushes a new stack frame with
an initially-null scopeChain, then creates the call object for the heavyweight
activation, then sets scopeChain = callobj.

The last patch ensures that cx->globalObject is a non-null fallback for use in
this case to find that "Call" has not been defined yet, and to find Function
(when doing js_InitCallClass => JS_InitClass => js_NewObject =>
js_GetClassPrototype => etc.) and Object if necessary.

I need to make one more patch, to close this hole in js_Invoke where
cx->globalObject ends up being used as a fallback because cx->fp->scopeChain is
momentarily null.  That's dynamic scoping, we can't have that.

/be

Comment 36

[Brendan Eich [:brendan]]

Attachment: really final patch -- stick a fork in me.

Comment 37

[Brendan Eich [:brendan]]

Attachment: better patch for just jscntxt.c, to fix independent bug found while developing the main patch

Comment 38

[Brendan Eich [:brendan]]

There was some bogus, unlikely to execute (but still broken) error recovery code
in js_NewContext.  I fixed the hell out of it.

/be

Comment 39

[Brendan Eich [:brendan]]

Ok, sorry for all the spam -- this is the message where I declare victory and
ask for testing and code review buddies.  I'd like to get this in today if it
passes muster.

/be

Comment 40

[rogerl (gone)]

Well to the extent that I was able to grok it all, (which isn't saying much I'm 
afraid) I think it's good to go. But whether that's a satisfying enough r= ...?

Comment 41

[Brendan Eich [:brendan]]

Attachment: consolidated final patch

Comment 42

[Brendan Eich [:brendan]]

Attachment: diff -wu version of consolidated final patch

Comment 43

[John Bandhauer]

r=jband.

I'm buying this all.

nits and trivial Qs...

- in jsnum the literal string removal left behind...
 if (!strncmp(istr, js_Infinity_str, 8))
...which didn't look as bad with the literal. Replace with 'sizeof(...)-1' ? 

- "((base &(base-1)) == 0" made me think a little. Old trick? I didn't figure 
out if this now allows base of 32 or 64 (etc) when it didn't before.??

- The 'Call' lookup fix is sneeking in? Did I hear a Tibet guy mention that 
today?

- Did you simulate errors to test the newcontext/destroycontext recovery? Or is 
this all run on the organic simulator? It looks to me like it came out ok.

- Also, you said you have a js.mak to fix the bustage? I guess we are still 
using pre-VC6 on these? I was going to add jsdhash.c myself but VC6 wanted to 
'upgrade' the project. So, I left it alone.

Check this stuff in!

Comment 44

[Brendan Eich [:brendan]]

> - in jsnum the literal string removal left behind...
> if (!strncmp(istr, js_Infinity_str, 8))
> ...which didn't look as bad with the literal. Replace with 'sizeof(...)-1' ? 

Oop, not my code, but now that you mention it, it should be using strcmp,
methinks.  Roger, is this right?  It shouldn't allow -InfinityFoo or InfinityBar
(trailing chars), eh?

> - "((base &(base-1)) == 0" made me think a little. Old trick? I didn't figure
> out if this now allows base of 32 or 64 (etc) when it didn't before.??

(I got a million of 'em.)  I ran this by rogerl -- callers constrain base to be
between 2 and 36, but the rounding fixup code here is needed for powers of two
anyway.

> - The 'Call' lookup fix is sneeking in? Did I hear a Tibet guy mention that
> today?

No sneak, this is a necessary fix given lazy class init.  See the patch with
comment "only 1 testsuite error with this patch" -- that was due to lack of
valid cx->globalObject or cx->fp->scopeChain when invoking a heavyweight
function and creating its Call object.

> - Did you simulate errors to test the newcontext/destroycontext recovery? Or
> is this all run on the organic simulator? It looks to me like it came out ok.

I simulated in gdb, it was too much for my old organic simulator.  Setting ok=0
after js_InitAtomState showed all the right cleanups happening (forced a silent
failure and 1 exit status from the shell, of course).

> - Also, you said you have a js.mak to fix the bustage? I guess we are still 
> using pre-VC6 on these? I was going to add jsdhash.c myself but VC6 wanted to 
> 'upgrade' the project. So, I left it alone.

I checked in the js.mak I had at home, which includes jsdhash rules and deps. 
Does it not work for you?  If not, please feel free to update it yourself -- I
am no MSVC project file guru.

> Check this stuff in!

Thanks!

On to the DOM, to actually use lazy JS_ResolveStandardClass instead of eager
JS_InitStandardClasses.

/be

Comment 45

[Brendan Eich [:brendan]]

Oop again, I didn't change that strncmp(istr, js_Infinity_str, 8) to a strcmp
after all (duh).  I did use sizeof(js_Infinity_str) - 1 for the length.  Thanks,
sorry for the bogus query, rogerl.

/be

Comment 46

[John Bandhauer]

>I checked in the js.mak I had at home, which includes jsdhash rules and deps. 
>Does it not work for you?  If not, please feel free to update it yourself -- I
>am no MSVC project file guru.

Yes, it works. I didn't realize you'd checked it in since we talked about it the 
other day.

Comment 47

[Brendan Eich [:brendan]]

Fix checked in.  Bug still open, onward to DOM victory.

/be

Comment 48

[Brendan Eich [:brendan]]

Attachment: proposed fix (nsGlobalWindow.cpp is not a complete patch, for review only)

Comment 49

[Brendan Eich [:brendan]]

This patch actually teaches the DOM to use lazy JS_ResolveStandardClass and
JS_EnumerateStandardClasses instead of eager JS_InitStandardClasses.  I couldn't
stand the travis-ties in nsGlobalWindow.cpp and gook GNU indent to it, then made
a pass by hand.  Full diffs (-u and -wu) coming soon, but don't melt your brain
too much on those -- the meat of this change is in the last patch.

/be

Comment 50

[Brendan Eich [:brendan]]

Attachment: diff -u format, valid patch so DOM uses lazy JS class init

Comment 51

[Brendan Eich [:brendan]]

Attachment: diff -wu format version of last patch

Comment 52

[Brendan Eich [:brendan]]

Looking for code buddies besides jband (jst this means you ;-).

/be

Comment 53

[rogerl (gone)]

I think that 'ok' in JS_ResolveStandardClass needs to be set to JS_TRUE if the 
init is not found - right now it's uninitialized.

Comment 54

[Brendan Eich [:brendan]]

Thanks, rogerl -- spot-fixing that blunder right now.

/be

Comment 55

[Brendan Eich [:brendan]]

GNU indent did a number on some declarations that I missed in the attached
patches, but have since fixed (like,

  nsXPIDLString
    name;

instead of

  nsXPIDLString name;

), all of which I have fixed.  Also, for those who don't know me and might fear
GNU indent, I used the right options (not GNU style, don't cuddle else with
prior }, etc.).  I am willing to take the cvs blame for nsGlobalWindow.cpp
rather than leave it to travis, because the bad formatting makes us all unhappy
and marginally less engaged and efficient with this code.

One of the first warning signs of bad ownership is conflicting or just wrong
style changes, but that implies there are such things as right style changes,
and I intend to check some in here!

Here are substantive changes I made that are hiding amid all the uninteresting
formatting changes in the diffs:

*** Removed unnecessary explicit assignment of nsnull to two nsCOMPtrs:

-GlobalWindowImpl::GlobalWindowImpl() : mScriptObject(nsnull),
+GlobalWindowImpl::GlobalWindowImpl() :
+  mScriptObject(nsnull),
    mNavigator(nsnull), mScreen(nsnull), mHistory(nsnull), mFrames(nsnull),
    mLocation(nsnull), mMenubar(nsnull), mToolbar(nsnull), mLocationbar(nsnull),
    mPersonalbar(nsnull), mStatusbar(nsnull), mScrollbars(nsnull),
@@ -130,8 +132,6 @@
    mChromeEventHandler(nsnull)
 {
    NS_INIT_REFCNT();
-   mCrypto = nsnull;
-   mPkcs11 = nsnull;
 }

*** my favorite useless if/else (or ?:, equivalently) elimination:

 NS_IMETHODIMP GlobalWindowImpl::GetClosed(PRBool* aClosed)
 {
-   if(!mDocShell)
-      *aClosed = PR_TRUE;
-   else 
-      *aClosed = PR_FALSE;
-
+  *aClosed = !mDocShell;
    return NS_OK;
 }

*** Use NS_SUCCEEDED(...) rather than NS_OK == ..., likewise for NS_FAILED(...)
    and NS_OK != ...

*** There was some very inefficient nsString/nsAutoString-based code to do with
    checking for an event handler being bound, looking for a property whose name
    starts with "on".  It would copy from JS's unicode vector string to a non
    auto nsString, then get its Length() and compare with 2, and if <= 2, then
    extract the first two PRUnichars into an nsAutoString and compare that to an
    inflated version of "on"!

    I recoded using direct loads of the first two jschars, comparing them to
    'o' and 'n' before bothering with making an nsString (which was needed to
    satisfy some other DOM or layout interface method).

*** Some of the nsIJSScriptObject interface method impls had inefficient code
    to handle string-valued properties: first checking for JSVAL_IS_STRING for
    all cases, then handling event handlers (function-valued props whose name
    begins with 'o', 'n', see above) consolidated the string-only logic.

    I hope vidur, scc, and jst don't hate me too much here!  Their branch for
    the new string interfaces will doubtless collide with my patch, but not in
    any deep way, I hope.  I'll help reconcile conflicts if needed.

Comment 56

[Brendan Eich [:brendan]]

So should I check in my nsGlobalWindow.cpp and nsJSEnvironment.cpp changes, or
what?  Getting antsy here....

/be

Comment 57

[Johnny Stenback (:jst)]

I'd say check it in! r=jst

Comment 58

[Brendan Eich [:brendan]]

Thanks, jst -- I just checked in dom/src/base nsGlobalWindow.cpp and
nsJSEnvironment.cpp.

Jband, you wanna do the rest of this (make the DOM classes initialize themselves
lazily)?  The nsIScriptNameSetRegistry/ScriptNameSpaceManager stuff might be the
way to go, it's already implemented in nsJSUtils::nsGlobalResolve.

/be

Comment 59

[Brendan Eich [:brendan]]

Trace-malloc analysis shows that prefs are still calling JS_InitStandardClasses
and being eager.  Here comes a patch that worksforme to make them lazy.  Someone
please r= me.

/be

Comment 60

[Brendan Eich [:brendan]]

Attachment: make prefs init JS standard classes lazily

Comment 61

[John Bandhauer]

looks right to me. r=jband

Comment 62

[Brendan Eich [:brendan]]

Prefs patch checked in, r=jband,blizzard.  Thanks, who wants to do the DOM
classes?  Jband, you wanna trade me this bug and you do the "turn off
JS_THREADSAFE" one?

/be

Comment 63

[Brendan Eich [:brendan]]

I'm trading with jband, so we don't get in the habit of fixing bugs on each
other's list.

/be

Comment 64

[Phil Peterson]

Not holding PR3 for this. Marking nsbeta3-. Please nominate for rtm if we need
to do this before we ship.

Comment 65

[Brendan Eich [:brendan]]

jst: I'm looking at my growing 0.9 buglist and hoping you'll take this bug back
and either fix nsJSContext::InitContext so it doesn't call InitClasses; or show
quantify data proving the eager DOM class init is not an issue, and therefore
mark this bug FIXED.  Can we declare victory?  Does XPConnected DOM change any
of this code?

/be

Comment 66

[Johnny Stenback (:jst)]

This should already be fixed on the XPCDOM branch, pushing to mozilla0.9.1

Comment 67

[Johnny Stenback (:jst)]

Fixed by the xpcdom landing.

Comment 68

[Prashant Desale]

[e-mail], or Johhney could you please verify this one ?

Comment 69

[Johnny Stenback (:jst)]

Verified.