Warren Harris Reporter
	Description • 25 years ago

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: jst@netscape.com
  Task owner: jband@netscape.com, vidur@netscape.com
  Status: JBand to determine the memory used by the core classes. JBand, Vidur 
and Johnny to determine what could break 
  with a super global.

	David Hyatt
	Comment 1 • 25 years ago

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

	John Bandhauer
	Comment 2 • 25 years ago

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.

	Warren Harris Reporter
	Comment 3 • 25 years ago

86k per window is small?

	John Bandhauer
	Comment 4 • 25 years ago

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.

	John Bandhauer
	Comment 5 • 25 years ago

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.

	Warren Harris Reporter
	Comment 6 • 25 years ago

You can do it!!!

	John Bandhauer
	Comment 7 • 25 years ago

warren's confidence is inspiring.

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

	Akkana Peck
	Comment 8 • 25 years ago

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!

	John Bandhauer
	Comment 9 • 24 years ago

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.

	Warren Harris Reporter
	Comment 10 • 24 years ago

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.

	Brendan Eich [:brendan]
	Comment 11 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 12 • 24 years ago

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

	Warren Harris Reporter
	Comment 13 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 14 • 24 years ago

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

	John Bandhauer
	Comment 15 • 24 years ago

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.

	Warren Harris Reporter
	Comment 16 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 17 • 24 years ago

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

	John Bandhauer
	Comment 18 • 24 years ago

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.

	Brendan Eich [:brendan]
	Comment 19 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 20 • 24 years ago

Changing subject to match reality.

/be

	Brendan Eich [:brendan]
	Comment 22 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 24 • 24 years ago

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

/be

	John Bandhauer
	Comment 26 • 24 years ago

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.

	Brendan Eich [:brendan]
	Comment 28 • 24 years ago

> 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

	Brendan Eich [:brendan]
	Comment 29 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 31 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 33 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 35 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 38 • 24 years ago

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

/be

	Brendan Eich [:brendan]
	Comment 39 • 24 years ago

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

	rogerl (gone)
	Comment 40 • 24 years ago

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= ...?

	John Bandhauer
	Comment 43 • 24 years ago

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!

	Brendan Eich [:brendan]
	Comment 44 • 24 years ago

> - 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

	Brendan Eich [:brendan]
	Comment 45 • 24 years ago

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

	John Bandhauer
	Comment 46 • 24 years ago

>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.

	Brendan Eich [:brendan]
	Comment 47 • 24 years ago

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

/be

	Brendan Eich [:brendan]
	Comment 49 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 52 • 24 years ago

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

/be

	rogerl (gone)
	Comment 53 • 24 years ago

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.

	Brendan Eich [:brendan]
	Comment 54 • 24 years ago

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

/be

	Brendan Eich [:brendan]
	Comment 55 • 24 years ago

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.

	Brendan Eich [:brendan]
	Comment 56 • 24 years ago

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

/be

	Johnny Stenback (:jst) Assignee
	Comment 57 • 24 years ago

I'd say check it in! r=jst

	Brendan Eich [:brendan]
	Comment 58 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 59 • 24 years ago

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

	John Bandhauer
	Comment 61 • 24 years ago

looks right to me. r=jband

	Brendan Eich [:brendan]
	Comment 62 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 63 • 24 years ago

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

/be

	Phil Peterson
	Comment 64 • 24 years ago

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

	Brendan Eich [:brendan]
	Comment 65 • 24 years ago

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

	Johnny Stenback (:jst) Assignee
	Comment 66 • 24 years ago

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

	Johnny Stenback (:jst) Assignee
	Comment 67 • 24 years ago

Fixed by the xpcdom landing.

	Prashant Desale
	Comment 68 • 24 years ago

warren@zodiacnetworks.com, or Johhney could you please verify this one ?

	Johnny Stenback (:jst) Assignee
	Comment 69 • 24 years ago

Verified.