373175 - Avoid arena naming code when JS_ARENAMETER is not defined

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently SpiderMonkey always passes some C string as the name argument to JS_InitArenaPool even if the argument is used only when JS_ARENAMETER is defined. In few cases the C string comes from JS_GetFunctionName that deflates the function name leading to creation of unused temporary objects.

It would be nice if the naming code would only present when JS_ARENAMETER is defined.

Comment 1

[Igor Bukanov]

Attachment: Implementation v1

The patch hides arena naming code behind a set of macros that expands to nothing with JS_ARENAMETER undefined. The patch adds %J support to JS_sprintf to print JSString* to avoid calling deprecated JS_GetFunctionName even in the debug code.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 257818 [details] [diff] [review]
Implementation v1

Shouldn't JS_InitArenaPool lose the name parameter, and JS_FinishArenaPool free and null pool->stats.name, if non-null?

A less invasive patch would leave the name parameter and all the calls that pass a constant string, and just address the few computed name cases. I'd prefer that.

/be

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> (From update of attachment 257818 [details] [diff] [review])
> Shouldn't JS_InitArenaPool lose the name parameter, 

But JS_InitArenaPool is a part of public API and can not be changed, can it?

> and JS_FinishArenaPool free
> and null pool->stats.name, if non-null?

Thanks for the catch.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 257818 [details] [diff] [review] [details])
> > Shouldn't JS_InitArenaPool lose the name parameter, 
> 
> But JS_InitArenaPool is a part of public API and can not be changed, can it?

No, it's not part of jsapi.h. The name says public, but that's just because it was forked from NSPR (NSPR 1, even). I think you could change it, but as noted I would rather just spot-fix the computed-name calls and leave the rest alone.

/be

Comment 5

[Brendan Eich [:brendan]]

ping...

Comment 6

[Igor Bukanov]

I will update the patch during weekend.

Comment 7

[Igor Bukanov]

Attachment: Implementation v2

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> Created an attachment (id=261005) [details]
> Implementation v2

The new patch replaces JS_InitArenaPool and js_NewPrinter by JS_INIT_ARENA_POOL and JS_NEW_PRINTER macros that removes the name argument when expanded without JS_ARENAMETER defined. To reduce the amount of #ifdef JS_ARENAMETER lines I use the macros both to call and define the functions.

To avoid using deprecated JS_GetFunctionName(fun) I replaced such calls by static strings for patch minimality. That can be restored if there are any value if such detailed arena naming.  

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 261005 [details] [diff] [review]
Implementation v2

Nice two-way macro-ization (calls and definition).

/be

Comment 10

[Igor Bukanov]

I committed the patch from comment 7 to the trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.311; previous revision: 3.310
done
Checking in jsarena.c;
/cvsroot/mozilla/js/src/jsarena.c,v  <--  jsarena.c
new revision: 3.33; previous revision: 3.32
done
Checking in jsarena.h;
/cvsroot/mozilla/js/src/jsarena.h,v  <--  jsarena.h
new revision: 3.25; previous revision: 3.24
done
Checking in jscntxt.c;
/cvsroot/mozilla/js/src/jscntxt.c,v  <--  jscntxt.c
new revision: 3.106; previous revision: 3.105
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.226; previous revision: 3.225
done
Checking in jsopcode.h;
/cvsroot/mozilla/js/src/jsopcode.h,v  <--  jsopcode.h
new revision: 3.44; previous revision: 3.43
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.277; previous revision: 3.276
done
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.144; previous revision: 3.143
done
Checking in jsscope.c;
/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.60; previous revision: 3.59
done