In many places SpiderMonkey calls directly or indirectly js_Atomize to get atoms for strings like "String", "Object" when in fact such atoms already available through JSRuntime.atomState. This extra atomization does not come free as the following example for js shell demonstrates: function test(N) { var time_function = Date.now; var str = "str"; var n = 0; var time = time_function(); while (N--) { n += str.length; } return time_function() - time; } var time = test(1000*1000); print(time); When I patch jsstr.c/jsobj.c so js_StringToObject from jsstr.c passes cx->runtime->atomState.StringAtom to a version of js_NewObject from jsobj.c that takes the atom as an argument, then the execution time of the script dropps by 10% from 1.1 to 1.0 second. This is for an optimized build of js shell under Ubuntu Linux with GCC 4.0 on 2.8 GHz on Pentium 4. So the idea is to use JSRuntime.atomState directly instead of atomizing C chars when possible.

Comment on attachment 210696 [details] [diff] [review] No atomization for js_ConstructObject users. This is the initial patch that changes js_ConstructObject to take extra atom argument and adjusts correspondingly its callers to either pass an atom from cx->runtime->atomState or atomize class' string (the single place in jsapi.c). The patch assumes that the patch from bug 321985 attachment 210695 [details] [diff] [review] is applied.

This is a htmlized version of the above test for the browser.

This is a full implementation. Besides changing js_ConstructObject to require an extra atom argument it introduces js_NewNamedObject. This is a version of js_NewObject that takes JSAtom* ctorName instead of prototype object. Then the patch switches all cases where js_NewObject was used with NULL prototype to use the new function. For that the patch added/moved few atoms to JSAtomState so they can be used in js_NewNamedObject. As with the previous patch this one also assumes that the patch from bug 321985 attachment 210695 [details] [diff] [review] is applied. I will ask for review when dependancy will be resolved.

It turns out I'm about to collide in bugzilla with this bug's patch, after I attach the patch I hacked last night to bug 326466. The goal of that patch is to implement Pythonic generators in SpiderMonkey, but it requires anonymous classes and class prototype invariance (see bug 304376). So in implementing generators I introduced a per-(cx x obj) prototype cache in a backward compatible fashion. Standard object instantiation thus needs no name or atom lookup to find the standard class's prototype. Igor, do you want to land this and I'll unconflict? Or are you willing to check out the patch I'll attach to bug 326466 and see whether it doesn't obviate the need for this bug's patch. /be

(In reply to comment #6) > Igor, do you want to land this and I'll unconflict? Or are you willing to > check out the patch I'll attach to bug 326466 and see whether it doesn't > obviate the need for this bug's patch. I can wait given that this patch depends on one from bug 321985 and I did not ask for review as this is not yet the final form in any case. But I would like to see the bug 321985 resolved.

(In reply to comment #7) > I can wait given that this patch depends on one from bug 321985 and I did not > ask for review as this is not yet the final form in any case. But I would like > to see the bug 321985 resolved. Yes, sorry for the delay there -- I'll review today. /be

not possible to automatically test relative performance improvements.

FYI, my main (Linux FC3) partition is badly corrupted and I have lost a lot of data. Limping on the WinXP partition (same disk) that seems to be working still. /be

This shouldn't be necessary for standard classes, given how js_ConstructObject calls js_GetClassId, which avoids atomization if JSCLASS_CACHED_PROTO_KEY(clasp) != JSProto_Null. What am I missing? /be

(In reply to comment #13) > This shouldn't be necessary for standard classes, given how js_ConstructObject > calls js_GetClassId, which avoids atomization if > JSCLASS_CACHED_PROTO_KEY(clasp) != JSProto_Null. What am I missing? Ok, *now* that bug 304376 is fixed, the "This shouldn't be necessary" claim ought to be true. Is it? /be

(In reply to comment #14) > Ok, *now* that bug 304376 is fixed, the "This shouldn't be necessary" claim > ought to be true. Is it? No, see bug 304376 comment 28. Time to find out what this patch optimizes that the patch from bug 304376 misses.

(In reply to comment #15) > (In reply to comment #14) > > Ok, *now* that bug 304376 is fixed, the "This shouldn't be necessary" claim > > ought to be true. Is it? > > No, see bug 304376 comment 28. Time to find out what this patch optimizes that > the patch from bug 304376 misses. Time for oprofile? /be

"perf" key word?

Isn't this bug invalid now, given jsproto.tbl and its use in js*.c? /be

I am not working on the bug right now.