Closed Bug 513530 Opened 10 years ago Closed 10 years ago

Cache result of Number2String

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: gal, Assigned: wagnerg)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files, 12 obsolete files)

This is a snapshot from xparb:

2017.000000
3.000000
54.000000
54.000000
2017.000000
9.000000
9.000000
4.000000
2017.000000
4.000000
19.000000
21.000000
2017.000000
9.000000
9.000000
5.000000
2017.000000
5.000000
43.000000
48.000000
2017.000000
8.000000

There is a lot of repeated number conversion, which is expensive (NewString, malloc), etc. We should cache here. Maybe 0..255 ? An IS_INTEGER check might help too.
Here is a proper histogram:

 137 0.000000
1065 1.000000
1036 10.000000
1006 11.000000
 920 12.000000
 399 13.000000
 403 14.000000
 397 15.000000
 396 16.000000
 386 17.000000
 396 18.000000
 393 19.000000
1044 2.000000
 391 20.000000
 674 2007.000000
 724 2008.000000
 721 2009.000000
 726 2010.000000
 730 2011.000000
 729 2012.000000
 719 2013.000000
 726 2014.000000
 728 2015.000000
 725 2016.000000
 497 2017.000000
 397 21.000000
 404 22.000000
 396 23.000000
 399 24.000000
 402 25.000000
 384 26.000000
 396 27.000000
 392 28.000000
 382 29.000000
1088 3.000000
 369 30.000000
 288 31.000000
 132 32.000000
 129 33.000000
 135 34.000000
 133 35.000000
 138 36.000000
 131 37.000000
 135 38.000000
 130 39.000000
1060 4.000000
 133 40.000000
 133 41.000000
 135 42.000000
 138 43.000000
 129 44.000000
 135 45.000000
 130 46.000000
 132 47.000000
 133 48.000000
 138 49.000000
1046 5.000000
 134 50.000000
 132 51.000000
 133 52.000000
 132 53.000000
 133 54.000000
 133 55.000000
 138 56.000000
 132 57.000000
 132 58.000000
 130 59.000000
1030 6.000000
1062 7.000000
1066 8.000000
1043 9.000000
shaver is suggesting doing the same for gmail. Assigning to gregor.
Assignee: general → gwagner
for the whole SS suite I see a total of 55401 js_NumberToString calls.
Int and within [0..255]: 39590, 71.460804%
and in histogram style
Attached image GMail results
Results for a short GMail session.
total js_NumberToString calls: 3986
INT and within 0..255: 2568 64.425489%
Love it. Patch soon? =)
Attached patch patch (obsolete) — Splinter Review
First version of numberToString cache.
Cache size is set to 65.
Performance numbers for patch v01:
TEST                   COMPARISON            FROM                 TO         

=============================================================================

** TOTAL **:           1.013x as fast    780.4ms +/- 0.1%   770.6ms +/- 0.2% 

=============================================================================

  3d:                  -                 118.0ms +/- 0.4%   117.9ms +/- 0.5% 
    cube:              ??                 33.4ms +/- 1.1%    33.5ms +/- 1.1% 
    morph:             -                  23.1ms +/- 1.0%    23.0ms +/- 0.0% 
    raytrace:          -                  61.5ms +/- 0.6%    61.4ms +/- 0.6% 

  access:              1.015x as fast    118.1ms +/- 0.6%   116.4ms +/- 0.3% 
    binary-trees:      1.021x as fast     33.4ms +/- 1.1%    32.7ms +/- 1.1% 
    fannkuch:          1.012x as fast     50.9ms +/- 0.4%    50.3ms +/- 0.7% 
    nbody:             -                  21.7ms +/- 1.6%    21.4ms +/- 1.7% 
    nsieve:            -                  12.1ms +/- 1.9%    12.0ms +/- 0.0% 

  bitops:              ??                 34.2ms +/- 2.6%    34.7ms +/- 1.0%
    3bit-bits-in-byte: ??                  1.6ms +/- 23.1%     1.7ms +/- 20.3% 
    bits-in-byte:      ??                 10.9ms +/- 2.1%    11.0ms +/- 0.0% 
    bitwise-and:       ??                  1.8ms +/- 16.7%     2.0ms +/- 0.0%
    nsieve-bits:       ??                 19.9ms +/- 1.1%    20.0ms +/- 0.0%

  controlflow:         -                  28.3ms +/- 1.2%    28.0ms +/- 0.0% 
    recursive:         -                  28.3ms +/- 1.2%    28.0ms +/- 0.0% 

  crypto:              -                  47.8ms +/- 2.3%    47.1ms +/- 1.8% 
    aes:               -                  27.6ms +/- 3.3%    27.0ms +/- 1.8% 
    md5:               -                  12.9ms +/- 1.8%    12.8ms +/- 2.4% 
    sha1:              -                   7.3ms +/- 4.7%     7.3ms +/- 4.7% 

  date:                1.035x as fast    113.1ms +/- 0.4%   109.3ms +/- 0.8%
    format-tofte:      1.011x as fast     57.3ms +/- 0.6%    56.7ms +/- 0.6%
    format-xparb:      1.061x as fast     55.8ms +/- 0.5%    52.6ms +/- 1.3%

  math:                ??                 25.4ms +/- 2.4%    25.9ms +/- 1.6%
    cordic:            ??                  8.3ms +/- 4.2%     8.6ms +/- 4.3%
    partial-sums:      ??                 11.8ms +/- 2.6%    12.0ms +/- 0.0% 
    spectral-norm:     -                   5.3ms +/- 6.5%     5.3ms +/- 6.5% 

  regexp:              -                  41.7ms +/- 0.8%    41.3ms +/- 0.8% 
    dna:               -                  41.7ms +/- 0.8%    41.3ms +/- 0.8% 

  string:              1.015x as fast    253.8ms +/- 0.3%   250.0ms +/- 0.4% 
    base64:            1.040x as fast     13.1ms +/- 1.7%    12.6ms +/- 2.9% 
    fasta:             ??                 52.6ms +/- 1.0%    53.0ms +/- 0.6% 
    tagcloud:          -                  81.1ms +/- 0.3%    80.9ms +/- 0.5% 
    unpack-code:       1.019x as fast     82.0ms +/- 0.4%    80.5ms +/- 0.6% 
    validate-input:    1.087x as fast     25.0ms +/- 0.0%    23.0ms +/- 1.5%
Comment on attachment 397918 [details] [diff] [review]
patch

>diff -r b18bb59b65ad js/src/jscntxt.h
>--- a/js/src/jscntxt.h	Mon Aug 31 12:11:00 2009 -0700
>+++ b/js/src/jscntxt.h	Tue Sep 01 11:51:04 2009 -0700
>@@ -488,16 +488,18 @@ struct JSRuntime {
> #endif
> 
>     /*
>      * Empty and unit-length strings held for use by this runtime's contexts.
>      * The unitStrings array and its elements are created on demand.
>      */
>     JSString            *emptyString;
>     JSString            **unitStrings;
>+    
>+    JSString            *numToStr[NUM_TO_STRING_CACHE_SIZE];

NUM_TO_STRING_CACHE_SIZE should be defined here, not in jsstr.h.

Nit: NUM_TO_STR_CACHE_SIZE to match member name. Or burn some more chars on numberToString and NUMBER_TO_STRING. Come to think of it, this is very concretely an INT_TO_STRING_CACHE so say that instead.

>+static void
>+MarkCachedStrings(JSRuntime *rt)
>+{
>+    for (int i = 0; i < NUM_TO_STRING_CACHE_SIZE; i++) {
>+        if (rt->numToStr[i]) {
>+            JSString *str = rt->numToStr[i];
>+            uint8 *flagp = GetGCThingFlags(str);
>+            JS_ASSERT((*flagp & GCF_TYPEMASK) == GCX_STRING);
>+            JS_ASSERT(*flagp != GCF_FINAL);
>+            *flagp |= GCF_MARK;

This should use JS_CallTracer and go through js_TraceRuntime, to match the tracing API.

But more important point: why mark these at all? We should collect any not in use by strong refs in the GC heap and root set. They will be regenerated on demand. I don't see a benchmarketing or other reason to hoard them till shutdown.

>+    
>+    if (gckind & GC_LAST_CONTEXT) {
>+         for (i = 0; i < NUM_TO_STRING_CACHE_SIZE; i++)
>+            cx->runtime->numToStr[i] = 0;

Nit: NULL not 0 -- but per above comment this code is not needed.

> JSString * JS_FASTCALL
> js_NumberToString(JSContext *cx, jsdouble d)
> {
>+    if (floor(d) == d) {

Use JSDOUBLE_IS_INT here, declaring jsint i beforehand at top level.

>+        int i = (int)d;
>+        if (i < NUM_TO_STRING_CACHE_SIZE && i >= 0) {
>+            if (cx->runtime->numToStr[i]) {
>+                return cx->runtime->numToStr[i];
>+            } else { 

else after return and trailing whitespace nit whambulance alert.

>+                cx->runtime->numToStr[i] = NumberToStringWithBase(cx, d, 10);
>+                return cx->runtime->numToStr[i];

This will leak if there are races (JS_THREADSAFE). Guess that's ok -- let the GC collect the losers. (Per my recommendation above let the GC collect 'em all!)

/be
Great win -- but why not make the cache size 256?

/be
Attached patch new patch (obsolete) — Splinter Review
Changed cache size to 256
Changed from all-marking to allow-gc approach.
Attachment #397918 - Attachment is obsolete: true
Attachment #397943 - Flags: review?(gal)
Comment on attachment 397943 [details] [diff] [review]
new patch

>diff -r b18bb59b65ad js/src/jscntxt.h
>--- a/js/src/jscntxt.h	Mon Aug 31 12:11:00 2009 -0700
>+++ b/js/src/jscntxt.h	Tue Sep 01 13:27:54 2009 -0700
>@@ -488,16 +488,19 @@ struct JSRuntime {
> #endif
> 
>     /*
>      * Empty and unit-length strings held for use by this runtime's contexts.
>      * The unitStrings array and its elements are created on demand.
>      */
>     JSString            *emptyString;
>     JSString            **unitStrings;
>+    
>+#define INT_TO_STRING_CACHE_SIZE     256
>+    JSString            *intToStr[INT_TO_STRING_CACHE_SIZE];

Use **numStrings like **unitStrings and malloc it like unitStrings, for consistency. NUM_STRING_CACHE_SIZE.

> 
>     /*
>      * Builtin functions, lazily created and held for use by the trace recorder.
>      *
>      * This field would be #ifdef JS_TRACER, but XPConnect is compiled without
>      * -DJS_TRACER and includes this header.
>      */
>     JSObject            *builtinFunctions[JSBUILTIN_LIMIT];
>diff -r b18bb59b65ad js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp	Mon Aug 31 12:11:00 2009 -0700
>+++ b/js/src/jsgc.cpp	Tue Sep 01 13:27:54 2009 -0700
>@@ -3552,16 +3552,22 @@ js_GC(JSContext *cx, JSGCInvocationKind 
>      * We first sweep atom state so we can use js_IsAboutToBeFinalized on
>      * JSString or jsdouble held in a hashtable to check if the hashtable
>      * entry can be freed. Note that even after the entry is freed, JSObject
>      * finalizers can continue to access the corresponding jsdouble* and
>      * JSString* assuming that they are unique. This works since the
>      * atomization API must not be called during GC.
>      */
>     js_SweepAtomState(cx);
>+    
>+    for (i = 0; i < INT_TO_STRING_CACHE_SIZE; i++) {
>+        if(rt->intToStr[i])

space between if and (

>+            if(js_IsAboutToBeFinalized(cx, rt->intToStr[i]))
>+                rt->intToStr[i] = NULL;
>+    }

Get a local pointer instead of rt->foo all the time.

> 
>     /* Finalize iterator states before the objects they iterate over. */
>     CloseNativeIterators(cx);
> 
>     /* Finalize watch points associated with unreachable objects. */
>     js_SweepWatchPoints(cx);
> 
> #ifdef DEBUG
>diff -r b18bb59b65ad js/src/jsnum.cpp
>--- a/js/src/jsnum.cpp	Mon Aug 31 12:11:00 2009 -0700
>+++ b/js/src/jsnum.cpp	Tue Sep 01 13:27:54 2009 -0700
>@@ -856,16 +856,24 @@ NumberToStringWithBase(JSContext *cx, js
>     if (!(numStr >= buf && numStr < buf + sizeof buf))
>         js_free(numStr);
>     return s;
> }
> 
> JSString * JS_FASTCALL
> js_NumberToString(JSContext *cx, jsdouble d)
> {
>+    jsint i;
>+    if (JSDOUBLE_IS_INT(d, i)) {
>+        if (i < INT_TO_STRING_CACHE_SIZE && i >= 0) {

Left to right. i >= 0 && i < MAX_CACHED_NUM_STRING.

Grab a pointer to &intToStr[i]

>+            if (!cx->runtime->intToStr[i])
>+                cx->runtime->intToStr[i] = NumberToStringWithBase(cx, d, 10);
>+            return cx->runtime->intToStr[i];

Look at the locking for unit strings.

>+        }
>+    }
>     return NumberToStringWithBase(cx, d, 10);
> }
> 
> JSBool JS_FASTCALL
> js_NumberValueToCharBuffer(JSContext *cx, jsval v, JSCharBuffer &cb)
> {
>     /* Convert to C-string. */
>     static const size_t arrSize = DTOSTR_STANDARD_BUFFER_SIZE;
Attached patch Version 3 (obsolete) — Splinter Review
Attachment #397943 - Attachment is obsolete: true
Attachment #397995 - Flags: review?(gal)
Attachment #397943 - Flags: review?(gal)
I like intStrings to parallel unitStrings, but let's make both inline array data members of JSRuntime and avoid the allocation and locking overhead.

/be
Although, you know... uintStrings would be a great typo-attractor for unitStrings (I'm sooo mean).

/be
Comment on attachment 397995 [details] [diff] [review]
Version 3

>+#define INT_TO_STRING_CACHE_SIZE     256
>+    JSString            **intToStr;

Blank line between #define and member. Also, could be all modern and use a scoped enum.

/be
Attached patch Version 4 (obsolete) — Splinter Review
changed intStrings and unitStrings :) to inline array data members of JSRuntime.
Attachment #397995 - Attachment is obsolete: true
Attachment #397995 - Flags: review?(gal)
Attached patch minor fix. combine enums (obsolete) — Splinter Review
Attachment #398066 - Attachment is obsolete: true
Comment on attachment 398171 [details] [diff] [review]
minor fix. combine enums

>     /*
>      * Empty and unit-length strings held for use by this runtime's contexts.
>-     * The unitStrings array and its elements are created on demand.
>      */
>+    enum {UNIT_STRING_SIZE = UNIT_STRING_LIMIT * sizeof(JSString *) +
>+                                 UNIT_STRING_LIMIT * 2 * sizeof(jschar), 
>+          INT_STRING_SIZE  = 256};

Standard style is

    enum {
        FOO = ...,
        BAR = A very long enumerator initializer
              wraps to underhang like so ...,
        BAZ = ...,
    };

but UNIT_STRING_SIZE is wrong -- the array dimention here:

>+    JSString            *unitStrings[UNIT_STRING_SIZE];

should be UNIT_STRING_LIMIT and declare a separate unitStringSpace array. Or else, if you want to keep the single allocation currently done by calloc, you'd want unitStrings to be of uint8 type.

If you using uint8 unitStringSpace[UNIT_STRING_SIZE] then the _SIZE suffix works, but indexing to fetch a unit string for a given char is uglier (needs an inline helper). So if possible use the type system and separate unitStrings from unitStringSpace.

>+    JSString            *intStrings[INT_STRING_SIZE];

Use LIMIT not SIZE for intString's dimension name-stem, since we try to keep size/SIZE for byte (sizeof-size) count, and to match UNIT_STRING_LIMIT. The LIMIT connotes fencepost or "do not equal or exceed" for indexes into the array.

/be
Attached patch Version 5 (obsolete) — Splinter Review
Yeah all the unitString, unitStringSpace and intStrings confused me...
I added a separate unitStringSpace array and adapted the size and limit constants.
Attachment #398171 - Attachment is obsolete: true
Attachment #398264 - Flags: review?(brendan)
Attachment #398264 - Attachment is obsolete: true
Attachment #398264 - Flags: review?(brendan)
Comment on attachment 398267 [details] [diff] [review]
minor fix. UNIT_STRING_SPACE_COUNT -> UNIT_STRING_SPACE_LIMIT

>+    js_InitUnitStringSpace(rt);

Finally I remembered shaver has a patch to use a single static (truly C/C++ static) array for the unit strings. Why not? They are not writable. See bug 491170.

>-        for (uintN i = 0; i < UNIT_STRING_LIMIT; i++) {
>+        for (uintN i = 0; i < rt->UNIT_STRING_LIMIT; i++) {

Longer but preferred way: JSRuntime::UNIT_STRING_LIMIT.

>-        if (c < UNIT_STRING_LIMIT) {
>+        if (c < cx->runtime->UNIT_STRING_LIMIT) {

Here JSRuntime:: wins by three characters over cx->runtime-> ;-).

>     /*
>      * Empty and unit-length strings held for use by this runtime's contexts.
>-     * The unitStrings array and its elements are created on demand.
>      */
>+    enum {
>+        UNIT_STRING_LIMIT        = 256U,
>+        UNIT_STRING_SPACE_LIMIT  = UNIT_STRING_LIMIT * 2, 
>+        INT_STRING_LIMIT         = 256U,
>+        UNIT_STRING_SPACE_SIZE   = UNIT_STRING_SPACE_LIMIT * sizeof(jschar)
>+    };
>+
>     JSString            *emptyString;
>-    JSString            **unitStrings;
>+    JSString            *unitStrings[UNIT_STRING_LIMIT];
>+    jschar              *unitStringSpace[UNIT_STRING_SPACE_LIMIT];

Wrong type, this should be jschar[], not jschar *[]. Casts hide the bug. This means you do not need UNIT_STRING_SPACE_SIZE.

>@@ -3283,16 +3283,17 @@ js_GC(JSContext *cx, JSGCInvocationKind 
>     uintN i, type;
>     JSTracer trc;
>     uint32 thingSize, indexLimit;
>     JSGCArenaInfo *a, **ap, *emptyArenas;
>     uint8 flags, *flagp;
>     JSGCThing *thing, *freeList;
>     JSGCArenaList *arenaList;
>     JSBool allClear;
>+    JSString **str;

Push this down to block scope.

Name should be strp, since it is double-indirect.

> JSString * JS_FASTCALL
> js_NumberToString(JSContext *cx, jsdouble d)
> {
>+    jsint i;
>+    JSString **str;
>+    JSRuntime *rt = cx->runtime;

Push str (rename to strp) and rt down to block needing them.

Could use JSString *&str if you can initialize the declaration, but we prefer explicit indirection over references for such things.

> String_fromCharCode(JSContext* cx, int32 i)
> {
>     JS_ASSERT(JS_ON_TRACE(cx));
>     jschar c = (jschar)i;
>-    if (c < UNIT_STRING_LIMIT)
>+    if (c < cx->runtime->UNIT_STRING_LIMIT)

JSRuntime:: again.

>+js_InitUnitStringSpace(JSRuntime *rt) {

Brace on its own line.

>+    jschar *cp = (jschar *)rt->unitStringSpace;

No cast here.

>+    for (int i = 0; i < rt->UNIT_STRING_LIMIT; i++) {
>+        *cp = i;
>+        cp += 2;
>+    }

But of course shaver's approach in bug 491170 seems better -- static initialization.

> js_MakeUnitString(JSContext *cx, jschar c)
> {
>+    jschar *cp;
>+    JSRuntime *rt = cx->runtime;;
>+    JS_ASSERT(c < rt->UNIT_STRING_LIMIT);
>+
>     if (!rt->unitStrings[c]) {
>         JSString *str;
> 
>-        cp = UNIT_STRING_SPACE_RT(rt);
>+        cp = (jschar *)rt->unitStringSpace;

Push cp declaration down to nearest block that needs it.

>-#define UNIT_STRING_LIMIT 256U
>+//#define UNIT_STRING_LIMIT 256U

Remove this and the blank line separator associated with it, hg will remember.

> #define IN_UNIT_STRING_SPACE_RT(rt,cp)                                        \
>+    ((size_t)((cp) - (jschar *)rt->unitStringSpace) < rt->UNIT_STRING_SPACE_SIZE)

Use size_t(...) style cast (pre-existing nit, we used to be C but we are now C++). But uintptr_t is even better.

Remove the (jschar *) cast.

JSRuntime:: not rt-> (although it might make for a longer line even with the (jschar *) removal.

> JSString::getUnitString(JSContext *cx, jschar c)
> {
>-    JS_ASSERT(c < UNIT_STRING_LIMIT);
>     JSRuntime *rt = cx->runtime;
>+    JS_ASSERT(c < cx->runtime->UNIT_STRING_LIMIT);

No need to move the assertion if you use JSRuntime:: not rt->.

> JSString::getUnitString(JSContext *cx, JSString *str, size_t index)
> {
>     JS_ASSERT(index < str->length());
>     jschar c = str->chars()[index];
>-    if (c >= UNIT_STRING_LIMIT)
>+    if (c >= cx->runtime->UNIT_STRING_LIMIT)

Ditto.

Really should have remembered shaver's bug 491170 before, sorry. Still seems worth investigating.

/be
Blocks: 491170
Attached patch Merge with patch from bug 491170 (obsolete) — Splinter Review
Now with single static array for unit strings.
Attachment #398267 - Attachment is obsolete: true
Blocks: 505818
Attachment #398473 - Attachment is obsolete: true
Attachment #398528 - Attachment is obsolete: true
Comment on attachment 398542 [details] [diff] [review]
Improved version, intStrings share one array

> 
>     if (str->length() == 1) {
>         jschar c = str->chars()[0];
>-        if (c < UNIT_STRING_LIMIT) {
>+        if (c < JSRuntime::UNIT_STRING_LIMIT) {
>             JSString *str = JSString::getUnitString(cx, c);
>             return str ? (JSAtom *) STRING_TO_JSVAL(str) : NULL;

This can no longer fail so just return (JSAtom *) STRONG_TO_JSVAL(JSString::getUnitString(c)); Also we don't need the context in ther any more.

>         jschar c = *chars;
>-        if (c < UNIT_STRING_LIMIT) {
>+        if (c < JSRuntime::UNIT_STRING_LIMIT) {
>             JSString *str = JSString::getUnitString(cx, c);

Same here.

>             return str ? (JSAtom *) STRING_TO_JSVAL(str) : NULL;
>         }
>     }
> 
>     str.initFlat((jschar *)chars, length);
>     state = &cx->runtime->atomState;
> 
>diff -r b18bb59b65ad js/src/jscntxt.h
>--- a/js/src/jscntxt.h	Mon Aug 31 12:11:00 2009 -0700
>+++ b/js/src/jscntxt.h	Thu Sep 03 18:16:26 2009 -0700
>@@ -484,20 +484,23 @@ struct JSRuntime {
> #endif
>     JSHashTable         *deflatedStringCache;
> #ifdef DEBUG
>     uint32              deflatedStringCacheBytes;
> #endif
> 
>     /*
>      * Empty and unit-length strings held for use by this runtime's contexts.
>-     * The unitStrings array and its elements are created on demand.
>      */
>+    enum {
>+        UNIT_STRING_LIMIT        = 256U,
>+        INT_STRING_LIMIT         = 256U,
>+    };
>+
>     JSString            *emptyString;
>-    JSString            **unitStrings;
> 
>     /*
>      * Builtin functions, lazily created and held for use by the trace recorder.
>      *
>      * This field would be #ifdef JS_TRACER, but XPConnect is compiled without
>      * -DJS_TRACER and includes this header.
>      */
>     JSObject            *builtinFunctions[JSBUILTIN_LIMIT];
>diff -r b18bb59b65ad js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp	Mon Aug 31 12:11:00 2009 -0700
>+++ b/js/src/jsgc.cpp	Thu Sep 03 18:16:26 2009 -0700
>@@ -2633,16 +2633,18 @@ JS_CallTracer(JSTracer *trc, void *thing
>             a->u.hasMarkedDoubles = JS_TRUE;
>         }
>         index = DOUBLE_THING_TO_INDEX(thing);
>         JS_SET_BIT(DOUBLE_ARENA_BITMAP(a), index);
>         goto out;
> 
>       case JSTRACE_STRING:
>         for (;;) {
>+            if (JSSTRING_IS_STATIC((JSString *)thing))
>+                goto out;
>             flagp = THING_TO_FLAGP(thing, sizeof(JSGCThing));
>             JS_ASSERT((*flagp & GCF_FINAL) == 0);
>             JS_ASSERT(kind == MapGCFlagsToTraceKind(*flagp));
>             if (!((JSString *) thing)->isDependent()) {
>                 *flagp |= GCF_MARK;
>                 goto out;
>             }
>             if (*flagp & GCF_MARK)
>@@ -3239,21 +3241,17 @@ js_FinalizeStringRT(JSRuntime *rt, JSStr
>         JS_ASSERT(str->dependentBase());
>         JS_RUNTIME_UNMETER(rt, liveDependentStrings);
>         valid = JS_TRUE;
>     } else {
>         /* A stillborn string has null chars, so is not valid. */
>         chars = str->flatChars();
>         valid = (chars != NULL);
>         if (valid) {
>-            if (IN_UNIT_STRING_SPACE_RT(rt, chars)) {
>-                JS_ASSERT(rt->unitStrings[*chars] == str);
>-                JS_ASSERT(type < 0);
>-                rt->unitStrings[*chars] = NULL;
>-            } else if (type < 0) {
>+            if (type < 0) {
>                 if (cx)
>                     cx->free(chars);
>                 else
>                     rt->free(chars);
>             } else {
>                 JS_ASSERT((uintN) type < JS_ARRAY_LENGTH(str_finalizers));
>                 finalizer = str_finalizers[type];
>                 if (finalizer) {
>diff -r b18bb59b65ad js/src/jsnum.cpp
>--- a/js/src/jsnum.cpp	Mon Aug 31 12:11:00 2009 -0700
>+++ b/js/src/jsnum.cpp	Thu Sep 03 18:16:26 2009 -0700
>@@ -856,16 +856,23 @@ NumberToStringWithBase(JSContext *cx, js
>     if (!(numStr >= buf && numStr < buf + sizeof buf))
>         js_free(numStr);
>     return s;
> }
> 
> JSString * JS_FASTCALL
> js_NumberToString(JSContext *cx, jsdouble d)
> {
>+    jsint i;
>+
>+    if (JSDOUBLE_IS_INT(d, i)) {
>+        if (i >= 0 && i < JSRuntime::INT_STRING_LIMIT) {

Brendan probably prefers if (jsuint(i) < JSRuntime::INT_STRING_LIMIT).

>+            return &js_IntStrings[i];
>+        }
>+    }
>     return NumberToStringWithBase(cx, d, 10);
> }

I will make a separate patch to make this inline in LIR.
Attached patch cleaned version (obsolete) — Splinter Review
added jsstatic.cpp file.
need proper place to include it.
Attachment #398542 - Attachment is obsolete: true
Comment on attachment 398551 [details] [diff] [review]
cleaned version


>@@ -683,20 +668,19 @@ js_AtomizeString(JSContext *cx, JSString
>     JSString *key;
>     uint32 gen;
> 
>     JS_ASSERT(!(flags & ~(ATOM_PINNED|ATOM_INTERNED|ATOM_TMPSTR|ATOM_NOCOPY)));
>     JS_ASSERT_IF(flags & ATOM_NOCOPY, flags & ATOM_TMPSTR);
> 
>     if (str->length() == 1) {
>         jschar c = str->chars()[0];
>-        if (c < UNIT_STRING_LIMIT) {
>-            JSString *str = JSString::getUnitString(cx, c);
>-            return str ? (JSAtom *) STRING_TO_JSVAL(str) : NULL;
>-        }
>+        if (c < JSRuntime::UNIT_STRING_LIMIT) {
>+             return (JSAtom *) STRING_TO_JSVAL(JSString::getUnitString(c));
>+         }

Nits:
* don't over-brace one-line consequents where the if (condition) is also one line;
* indentation is off by one (too far) on the second and third + lines.

>+        if (c < JSRuntime::UNIT_STRING_LIMIT) {
>+            return (JSAtom *)STRING_TO_JSVAL(JSString::getUnitString(c));
>         }

Ditto the over-bracing nit.

>@@ -484,20 +484,23 @@ struct JSRuntime {
> #endif
>     JSHashTable         *deflatedStringCache;
> #ifdef DEBUG
>     uint32              deflatedStringCacheBytes;
> #endif
> 
>     /*
>      * Empty and unit-length strings held for use by this runtime's contexts.
>-     * The unitStrings array and its elements are created on demand.
>      */
>+    enum {
>+        UNIT_STRING_LIMIT        = 256U,
>+        INT_STRING_LIMIT         = 256U,
>+    };
>+

Now that we have static global tables, these do not need to go in JSRuntime. Better in jsstr.h as before, minimizes the patch.

>     JSString            *emptyString;
>-    JSString            **unitStrings;

But of course we still lose this JSRuntime member, pure win.

>@@ -2633,16 +2633,18 @@ JS_CallTracer(JSTracer *trc, void *thing
>             a->u.hasMarkedDoubles = JS_TRUE;
>         }
>         index = DOUBLE_THING_TO_INDEX(thing);
>         JS_SET_BIT(DOUBLE_ARENA_BITMAP(a), index);
>         goto out;
> 
>       case JSTRACE_STRING:
>         for (;;) {
>+            if (js_stringIsStatic((JSString *)thing))

Naming convention wants js_StringIsStatic (js_StudlyCaps) but why not make it a static JSString method?

>@@ -3239,21 +3241,17 @@ js_FinalizeStringRT(JSRuntime *rt, JSStr
>         JS_ASSERT(str->dependentBase());
>         JS_RUNTIME_UNMETER(rt, liveDependentStrings);
>         valid = JS_TRUE;
>     } else {
>         /* A stillborn string has null chars, so is not valid. */
>         chars = str->flatChars();
>         valid = (chars != NULL);
>         if (valid) {
>-            if (IN_UNIT_STRING_SPACE_RT(rt, chars)) {
>-                JS_ASSERT(rt->unitStrings[*chars] == str);
>-                JS_ASSERT(type < 0);
>-                rt->unitStrings[*chars] = NULL;
>-            } else if (type < 0) {
>+            if (type < 0) {

Should assert !js_StringIsStatic(str) hereabouts, or at the top of js_FinalizeStringRT.

> js_NumberToString(JSContext *cx, jsdouble d)
> {
>+    jsint i;
>+
>+    if (JSDOUBLE_IS_INT(d, i)) {
>+        if (i >= 0 && jsuint(i) < JSRuntime::INT_STRING_LIMIT) {
>+            return &js_IntStrings[i];
>+        }

Don't over-brace nit again.

>+++ b/js/src/jsstatic.cpp	Thu Sep 03 19:15:00 2009 -0700

Not sure why Andreas wanted a new file. Its name is a bit overloaded, and it could become a dumping ground for static data that should go in other files (anti-modularity attractive nuisance). I would have thought this data fit in jsstr.cpp. Maybe there is some linker reason. Andreas?

>@@ -0,0 +1,206 @@
>+#define C(c) c, 0x00
>+#define D(c, d) c, d, 0x00
>+#define E(c, d, e) c, d, e, 0x00
>+
>+/*
>+ * String data for all unit strings (including zero-char backstop required for independent strings),
>+ * packed into single array.
>+ */
>+static const jschar
>+UnitStringData[] =
>+{

Nit: you can put the above three lines including the brace on one line, that is prevailing style for data definition with initializer.

>+    C(0x00), C(0x01), C(0x02), C(0x03), C(0x04), C(0x05), C(0x06), C(0x07), 

Trailing whitespace alert! Grep for it in your patch.

>+JSString
>+js_UnitStrings[] =
>+{

Ditto on these three fitting on one line.

>+static const jschar 
>+hundrets[] =
>+{

Ditto three lines to one, also "hundreds". May as well spell it "Hundreds" to match the first static table's StudlyCaps naming.

>+    O10(0x30), O10(0x31), O10(0x32), O10(0x33), O10(0x34), O10(0x35), O10(0x36), O10(0x37), O10(0x38), O10(0x39), 
>+    O11(0x30), O11(0x31), O11(0x32), O11(0x33), O11(0x34), O11(0x35), O11(0x36), O11(0x37), O11(0x38), O11(0x39), 
>+    O12(0x30), O12(0x31), O12(0x32), O12(0x33), O12(0x34), O12(0x35), O12(0x36), O12(0x37), O12(0x38), O12(0x39), 
>+    O13(0x30), O13(0x31), O13(0x32), O13(0x33), O13(0x34), O13(0x35), O13(0x36), O13(0x37), O13(0x38), O13(0x39),
>+    O14(0x30), O14(0x31), O14(0x32), O14(0x33), O14(0x34), O14(0x35), O14(0x36), O14(0x37), O14(0x38), O14(0x39), 
>+    O15(0x30), O15(0x31), O15(0x32), O15(0x33), O15(0x34), O15(0x35), O15(0x36), O15(0x37), O15(0x38), O15(0x39),
>+    O16(0x30), O16(0x31), O16(0x32), O16(0x33), O16(0x34), O16(0x35), O16(0x36), O16(0x37), O16(0x38), O16(0x39), 
>+    O17(0x30), O17(0x31), O17(0x32), O17(0x33), O17(0x34), O17(0x35), O17(0x36), O17(0x37), O17(0x38), O17(0x39),
>+    O18(0x30), O18(0x31), O18(0x32), O18(0x33), O18(0x34), O18(0x35), O18(0x36), O18(0x37), O18(0x38), O18(0x39),
>+    O19(0x30), O19(0x31), O19(0x32), O19(0x33), O19(0x34), O19(0x35), O19(0x36), O19(0x37), O19(0x38), O19(0x39),
>+    O20(0x30), O20(0x31), O20(0x32), O20(0x33), O20(0x34), O20(0x35), O20(0x36), O20(0x37), O20(0x38), O20(0x39), 
>+    O21(0x30), O21(0x31), O21(0x32), O21(0x33), O21(0x34), O21(0x35), O21(0x36), O21(0x37), O21(0x38), O21(0x39),
>+    O22(0x30), O22(0x31), O22(0x32), O22(0x33), O22(0x34), O22(0x35), O22(0x36), O22(0x37), O22(0x38), O22(0x39), 
>+    O23(0x30), O23(0x31), O23(0x32), O23(0x33), O23(0x34), O23(0x35), O23(0x36), O23(0x37), O23(0x38), O23(0x39),
>+    O24(0x30), O24(0x31), O24(0x32), O24(0x33), O24(0x34), O24(0x35), O24(0x36), O24(0x37), O24(0x38), O24(0x39),
>+    O25(0x30), O25(0x31), O25(0x32), O25(0x33), O25(0x34), O25(0x35)
>+};
>+
>+#define L1(c) { 1 | JSString::ATOMIZED, {(jschar *)hundrets + 2 + (c) * 4} } /* length 1: 0..9 */
>+#define L2(c) { 2 | JSString::ATOMIZED, {(jschar *)hundrets + 41 + (c - 10) * 4} } /* length 2: 10..99 */
>+#define L3(c) { 3 | JSString::ATOMIZED, {(jschar *)hundrets + (c - 100) * 4} } /* length 3: 100..255 */

Blank line here, and before if I missed a similar thing.

>+JSString
>+js_IntStrings[] =
>+{

Ditto three to one line reduction.

All the #undefs seem unnecessary if jsstatic.cpp or jsstaticdata.cpp survives, but if you put the table in jsstr.cpp they're ok.

>\ No newline at end of file

Please fix this.

>+JSBool 
>+js_stringIsStatic(JSString *s) 
>+{
>+    return ((s) > &js_UnitStrings[-1] && (s) < &js_UnitStrings[JSRuntime::UNIT_STRING_LIMIT]) ||
>+        ((s) > &js_IntStrings[-1]  && (s) < &js_IntStrings[JSRuntime::INT_STRING_LIMIT]);

Don't over-parentehsize (s).

Do use bool return type here. This is not a traceable native.

Use ... >= js_UnitStrings && ... < js_UnitStrings + UNIT_STRING_LIMIT, etc. or number line order, but don't use > &js_UnitStrings[-1], that's just confusing.

Can this not be inline in jsstr.h? It should be for the GC's benefit. I'm hoping it can be static bool JSString::isStatic and yet also be inline.

It'll be easier to read without the JSRuntime:: prefixing.

>-        (code = js_ValueToUint16(cx, &argv[0])) < UNIT_STRING_LIMIT) {
>-        str = JSString::getUnitString(cx, code);
>+        (code = js_ValueToUint16(cx, &argv[0])) < JSRuntime::UNIT_STRING_LIMIT) {
>+        str = JSString::getUnitString(code);

The JSRuntime:: elimination minimizes the diff.

> js_GetStringBytes(JSContext *cx, JSString *str)
> {
>     JSRuntime *rt;
>     JSHashTable *cache;
>     char *bytes;
>     JSHashNumber hash;
>     JSHashEntry *he, **hep;
> 
>+    if (js_stringIsStatic(str)) {
>+#ifdef IS_LITTLE_ENDIAN
>+        /* Unit string data is {c, 0, 0, 0} so we can just cast. */
>+        return (char *)str->chars();
>+#else
>+        /* Unit string data is {0, c, 0, 0} so we can point into the middle. */
>+        return (char *)str->chars() + 1;

You are a wildman. I'm digging this.

>+#endif            

Trailing whitespace alert! Get 'em all.

/be
Cleaned patch coming soon. First some performance numbers. The wildman title goes to Shaver :) Its from his original patch. 


TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           1.010x as fast    769.8ms +/- 0.2%   762.4ms +/- 0.2%     significant

=============================================================================

  3d:                  1.005x as fast    117.1ms +/- 0.3%   116.5ms +/- 0.3%     significant
    cube:              1.015x as fast     33.0ms +/- 0.0%    32.5ms +/- 1.2%     significant
    morph:             -                  23.2ms +/- 1.3%    23.0ms +/- 0.0% 
    raytrace:          ??                 60.9ms +/- 0.4%    61.0ms +/- 0.0%     not conclusive: might be *1.002x as slow*

  access:              *1.025x as slow*  115.4ms +/- 0.3%   118.3ms +/- 0.5%     significant
    binary-trees:      *1.053x as slow*   32.0ms +/- 0.0%    33.7ms +/- 1.0%     significant
    fannkuch:          *1.024x as slow*   50.0ms +/- 0.0%    51.2ms +/- 0.6%     significant
    nbody:             ??                 21.5ms +/- 1.8%    21.8ms +/- 1.4%     not conclusive: might be *1.014x as slow*
    nsieve:            -                  11.9ms +/- 1.9%    11.6ms +/- 3.2% 

  bitops:              *1.028x as slow*   32.2ms +/- 0.9%    33.1ms +/- 1.2%     significant
    3bit-bits-in-byte: -                   1.5ms +/- 25.1%     1.2ms +/- 25.1% 
    bits-in-byte:      *1.190x as slow*    8.4ms +/- 4.4%    10.0ms +/- 0.0%     significant
    bitwise-and:       -                   2.1ms +/- 10.8%     1.9ms +/- 11.9% 
    nsieve-bits:       -                  20.2ms +/- 1.5%    20.0ms +/- 0.0% 

  controlflow:         -                  28.4ms +/- 1.3%    28.3ms +/- 1.2% 
    recursive:         -                  28.4ms +/- 1.3%    28.3ms +/- 1.2% 

  crypto:              1.030x as fast     47.5ms +/- 2.2%    46.1ms +/- 1.9%     significant
    aes:               -                  27.5ms +/- 4.1%    26.3ms +/- 3.4% 
    md5:               1.049x as fast     12.8ms +/- 2.4%    12.2ms +/- 2.5%     significant
    sha1:              ??                  7.2ms +/- 4.2%     7.6ms +/- 4.9%     not conclusive: might be *1.056x as slow*

  date:                1.031x as fast    114.3ms +/- 0.3%   110.9ms +/- 0.4%     significant
    format-tofte:      1.017x as fast     58.2ms +/- 0.5%    57.2ms +/- 0.5%     significant
    format-xparb:      1.045x as fast     56.1ms +/- 0.4%    53.7ms +/- 0.6%     significant

  math:                -                  25.7ms +/- 2.3%    25.3ms +/- 2.3% 
    cordic:            1.086x as fast      8.8ms +/- 3.4%     8.1ms +/- 2.8%     significant
    partial-sums:      ??                 11.6ms +/- 3.2%    11.7ms +/- 3.0%     not conclusive: might be *1.009x as slow*
    spectral-norm:     ??                  5.3ms +/- 6.5%     5.5ms +/- 6.8%     not conclusive: might be *1.038x as slow*

  regexp:              1.021x as fast     34.7ms +/- 1.0%    34.0ms +/- 0.0%     significant
    dna:               1.021x as fast     34.7ms +/- 1.0%    34.0ms +/- 0.0%     significant

  string:              1.018x as fast    254.5ms +/- 0.3%   249.9ms +/- 0.2%     significant
    base64:            *1.015x as slow*   13.0ms +/- 0.0%    13.2ms +/- 2.3%     significant
    fasta:             1.020x as fast     55.5ms +/- 0.7%    54.4ms +/- 0.7%     significant
    tagcloud:          1.030x as fast     82.1ms +/- 0.5%    79.7ms +/- 0.4%     significant
    unpack-code:       *1.009x as slow*   78.9ms +/- 0.5%    79.6ms +/- 0.5%     significant
    validate-input:    1.087x as fast     25.0ms +/- 0.0%    23.0ms +/- 0.0%     significant
Attachment #398551 - Attachment is obsolete: true
Comment on attachment 398757 [details] [diff] [review]
new version. adding brendans suggestions

>@@ -683,20 +668,18 @@ js_AtomizeString(JSContext *cx, JSString
>     JSString *key;
>     uint32 gen;
> 
>     JS_ASSERT(!(flags & ~(ATOM_PINNED|ATOM_INTERNED|ATOM_TMPSTR|ATOM_NOCOPY)));
>     JS_ASSERT_IF(flags & ATOM_NOCOPY, flags & ATOM_TMPSTR);
> 
>     if (str->length() == 1) {
>         jschar c = str->chars()[0];
>-        if (c < UNIT_STRING_LIMIT) {
>-            JSString *str = JSString::getUnitString(cx, c);
>-            return str ? (JSAtom *) STRING_TO_JSVAL(str) : NULL;
>-        }
>+        if (c < UNIT_STRING_LIMIT)
>+            return (JSAtom *)STRING_TO_JSVAL(JSString::getUnitString(c));

Uber-nit: leave a space between the cast and its operand: (JSAtom *) STRING_TO_JSVAL(...);

> js_GetExistingStringAtom(JSContext *cx, const jschar *chars, size_t length)
> {
>     JSString str, *str2;
>     JSAtomState *state;
>     JSDHashEntryHdr *hdr;
> 
>     if (length == 1) {
>         jschar c = *chars;
>-        if (c < UNIT_STRING_LIMIT) {
>-            JSString *str = JSString::getUnitString(cx, c);
>-            return str ? (JSAtom *) STRING_TO_JSVAL(str) : NULL;
>-        }
>+        if (c < UNIT_STRING_LIMIT)
>+            return (JSAtom *)STRING_TO_JSVAL(JSString::getUnitString(c));

Ditto.

> js_NumberToString(JSContext *cx, jsdouble d)
> {
>+    jsint i;
>+
>+    if (JSDOUBLE_IS_INT(d, i)) {
>+        if (i >= 0 && jsuint(i) < INT_STRING_LIMIT)
>+            return &js_IntStrings[i];

You don't need the i >= 0 test given the jsuint(i) < INT_STRING_LIMIT test.

> inline JSString *
>-JSString::getUnitString(JSContext *cx, jschar c)
>+JSString::getUnitString(jschar c)
> {
>     JS_ASSERT(c < UNIT_STRING_LIMIT);
>+    return js_UnitStrings + c;
> }

Suggest not overloading getUnitString, rather use a naming convention we use elsewhere, where a noun phrase for a method name connotes infallibility: unitString(c).

This could be inlined in jsstr.h, but perhaps it should stay here, otherwise getUnitString(cx, str, index) will be lonely.

/be
Comment on attachment 398757 [details] [diff] [review]
new version. adding brendans suggestions

> inline JSString *
> JSString::getUnitString(JSContext *cx, JSString *str, size_t index)
> {
>     JS_ASSERT(index < str->length());
>     jschar c = str->chars()[index];
>     if (c >= UNIT_STRING_LIMIT)
>         return js_NewDependentString(cx, str, index, 1);
>-    return getUnitString(cx, c);
>+    return getUnitString(c);

Also, pre-existing code I know, but this might be better written

    if (c < UNIT_STRING_LIMIT)
        return unitString(c);
    return js_NewDependentString(cx, str, index, 1);

/be
Attached patch fixedSplinter Review
save the lonely functions!
Attachment #398757 - Attachment is obsolete: true
Comment on attachment 398773 [details] [diff] [review]
fixed

Nice, thanks! Get a checkin buddy from irc #jsapi and get this in.

/be
Attachment #398773 - Flags: review+
Followup to make the tables static members of JSString. I had hoped this would allow the JSString-element-typed tables to be initialized with curly-brace inits, but no dice. Stilll, it is tidier to use class statics, and perhaps there will be a way to initialize statically (anyone know of one? Maybe in C++0x?).

http://hg.mozilla.org/tracemonkey/rev/1f50d5f4d3a5

I should have pointed this approach out during review, sorry. r=gwagner and there is an unrelated warning fix (see bug 514454).

/be
Depends on: 514819
Duplicate of this bug: 491170
Depends on: 514971
looks like this broke maemo builds somehow. imacro generation is crashing.
Can you link the log for gregor? Do we have crash dumps o maemo?
hmm, no crash dump there, it's causing qemu to exit.

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1252108528.1252108859.12117.gz&fulltext=1

./../../dist/bin/run-mozilla.sh ./../../dist/bin/js imacro_asm.js /home/cltbld/build/tracemonkey/js/src/imacros.jsasm > imacros.c.tmp
qemu: uncaught target signal 11 (Segmentation fault) - exiting
(In reply to comment #41)
> hmm, no crash dump there, it's causing qemu to exit.
>
> ./../../dist/bin/run-mozilla.sh ./../../dist/bin/js imacro_asm.js
> /home/cltbld/build/tracemonkey/js/src/imacros.jsasm > imacros.c.tmp
> qemu: uncaught target signal 11 (Segmentation fault) - exiting

That looks just like what I saw when I tried to run 'js' on the Maemo/scratchbox installation on my x86-64 Linux box.  The bad news is I never worked out what the problem was :(
10.250.2.223 is an arm linux box. user mozilla. password firefox. You have to VPN into the office.
qemu supports something like armv4, typically interesting code is armv6. there are similar problems trying to use fennec in the windows mobile emulator.
I will build and debug on a real arm box today. My first guess is also a qemu problem.
Depends on: 515248
Depends on: 515273
http://hg.mozilla.org/mozilla-central/rev/0f106a0d31a1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 528645
You need to log in before you can comment on or make changes to this bug.