js_XDRStringAtom should stack allocate for the win

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Stuart Parmenter, Assigned: Stuart Parmenter)

Tracking

({memory-footprint, perf})

Trunk
x86
All
memory-footprint, perf
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 292158 [details] [diff] [review]
use the stack and JS_malloc rather than the tempPool arena

After using a 256 char stack buffer, with Firefox drops to only 2 actual allocations.  One 551 byte one and one 6053 byte one (from http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#6237 and http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsProxyAutoConfig.js#188)

Before I was seeing over 25000 1024 byte allocations. :/
Attachment #292158 - Flags: review?(igor)

Comment 1

11 years ago
Comment on attachment 292158 [details] [diff] [review]
use the stack and JS_malloc rather than the tempPool arena

>+    if (nchars < (sizeof(stackChars) / sizeof(jschar)))

JS_ARRAY_LENGTH(stackChars)

>+        chars = (jschar*)JS_malloc(cx, nchars * sizeof(jschar));

(jschar *) JS_malloc(...);

Comment 2

11 years ago
Fix the block-comments per my suggestion on IRC and (I think) do braces around the else clause, since the comment offsets the lone statement by so much.  Hard to find examples of this in spidermonkey, but we tend to err on the side of bracing rather than not where there seem to be questions.
The style rule is very clear, lots of precedent. If *any* of the if condition, the then clause, and the else clause, for any reason (including comment lines) takes more than one line, then both then and else clauses, or then if no else, must be braced.

/be
(Assignee)

Comment 4

11 years ago
Created attachment 292166 [details] [diff] [review]
fix style nits
Assignee: general → pavlov
Attachment #292158 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #292166 - Flags: review?(igor)
Attachment #292158 - Flags: review?(igor)
Comment on attachment 292166 [details] [diff] [review]
fix style nits

>+    if (nchars < JS_ARRAY_LENGTH(stackChars)) {

Drive-by: should be <= here.

>+        chars = stackChars;
>+    } else {
>+        /*
>+         * This is very uncommon. Don't use the tempPool arena for this
>+         * as most things we'll encounter here will be > 1024 bytes anyways

Period at end, wrap as if by Qj in vim with tw=78, and shorten to "as most allocations here will be bigger than tempPool's arenasize."

>+        chars = (jschar*) JS_malloc(cx, nchars * sizeof(jschar));

Nit: (jschar *) (space before *).

/be
(Assignee)

Comment 6

11 years ago
Created attachment 292189 [details] [diff] [review]
more nits...
Attachment #292166 - Attachment is obsolete: true
Attachment #292189 - Flags: review?(igor)
Attachment #292166 - Flags: review?(igor)
Comment on attachment 292189 [details] [diff] [review]
more nits...

Thanks for the fixes (the <= wasn't a nit :-P). r=me fwiw, happy to have Igor go over it too.

/be
Attachment #292189 - Flags: review+

Comment 8

11 years ago
Comment on attachment 292189 [details] [diff] [review]
more nits...

> extern JSBool
> js_XDRStringAtom(JSXDRState *xdr, JSAtom **atomp)
> {
...
>+        /*
>+         * This is very uncommon. Don't use the tempPool arena for this as
>+         * most allocations here will be bigger than tempPool's arenasize.
>+         */
>+        chars = (jschar *) JS_malloc(cx, nchars * sizeof(jschar));

Nit: early return here on out-of-memory to follow SM style and avoid unnecessary check for non-null chars in the common case of a small string. r+ with that fixed.
(Assignee)

Comment 9

11 years ago
checked in.  thanks for reviews
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-
(Assignee)

Updated

11 years ago
Attachment #292189 - Flags: review?(igor) → review+
(Assignee)

Updated

11 years ago
Keywords: footprint, perf
You need to log in before you can comment on or make changes to this bug.