540805 - move JSScope::nrefs to JSEmptyScope

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

JSScope::nrefs currently is only used to share empty and block scopes. For other scopes nrefs is one. This suggests to move nrefs to JSEmptyScope while switching the execution-time block scopes can be switched to use the same empty scopes that the rest of scopes use. This would make the scope learner and removes some check from the object finalizer.

Comment 1

[Igor Bukanov]

Attachment: implementation v1

The patch moves nrefs from JSScope to JSEmptyScope and optimizes empty scope creation and its transition into mutable one. As far as I can see it does not affect SunSpider and other benchmarks, but it does show a win for the following synthetic one:

function test(N)
{
    var N = 1e6;
    var a = [];
    var time0 = Date.now();
    for(var i = 0; i != N; ++i) {
        a[i] = {a: 1};
    }
    var time0 = Date.now() - time0;
    a = null;
    var time1 = Date.now();
    gc();
    time1 = Date.now() - time1;
    var a = [];
    for(var i = 0; i != N; ++i) {
        a[i] = new Object();
    }
    a = null;
    var time2 = Date.now();
    gc();
    time2 = Date.now() - time2;
    return [time0, time1, time2];
}

// warmup
test();


var min_times = [Infinity, Infinity, Infinity];

for (var i = 0; i != 10; ++i) {
    var times = test();
    min_times[0] = Math.min(min_times[0], times[0]);
    min_times[1] = Math.min(min_times[1], times[1]);
    min_times[2] = Math.min(min_times[2], times[2]);
}
print("Non empty scope allocation: "+min_times[0]);
print("Non empty scope finalization: "+min_times[1]);
print("Empty scope finalization: "+min_times[2]);


The patch changes timing for a typical -j run on 2.6Ghz Xenon from:

Non empty scope allocation: 254
Non empty scope finalization: 97
Empty scope finalization: 42

to:

Non empty scope allocation: 254
Non empty scope finalization: 86
Empty scope finalization: 42

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 423258 [details] [diff] [review]
implementation v1

In jsscope.h, struct JSScope:
>     uint16          spare;              /* reserved */
>     uint32          entryCount;         /* number of entries in table */
>     uint32          removedCount;       /* removed entry sentinels in table */
>+    JSEmptyScope    *emptyScope;        /* cache for getEmptyScope below */
>     JSScopeProperty **table;            /* table of ptrs to shared tree nodes */

I don't mind moving emptyScope, but please move it after table since entryCount,
removedCount, and table are closely related.

>-    explicit JSScope(const JSObjectOps *ops, JSObject *obj = NULL)
>+    explicit JSScope(const JSObjectOps *ops, JSObject *obj)
>       : JSObjectMap(ops, 0), object(obj) {}

`explicit` doesn't do anything here. Luke points out that it might matter in
combination with new C++0x features, but let's delete it for now.

In JSEmptyScope:
>-    explicit JSEmptyScope(const JSObjectOps *ops, JSClass *clasp)
>-      : JSScope(ops), clasp(clasp) {}
>+    explicit JSEmptyScope(JSContext *cx, const JSObjectOps *ops,
>+                          JSClass *clasp);
>+

Also here.

>+    inline void hold() {
>+        /* The method is only called for already held objects. */
>+        JS_ASSERT(nrefs >= 1);
>+        JS_ATOMIC_INCREMENT(&nrefs);
>+    }

`inline` is redundant here.

In jsscopeinlines.h:
>+inline JSEmptyScope *
>+JSScope::createEmptyScope(JSContext *cx, JSClass *clasp)
>+{
>+    JS_ASSERT(!emptyScope);
>+
>+    JSEmptyScope *scope = cx->create<JSEmptyScope>(cx, ops, clasp);
>+    if (!scope)
>+        return NULL;
>+    emptyScope = scope;
>+    return scope;
>+}

It could just say,

    emptyScope = cx->create<JSEmptyScope>(cx, ops, clasp);
    return emptyScope;

In jslock.h:
>+#define JS_OWN_SCOPE(cx,scope)     ((scope)->title.ownercx == (cx))

I don't think factoring this code out into a macro makes the code any
clearer. The name is misleading, since "owning" a scope is likely to be
interpreted as holding the scope lock.

Nice patch. r=me with nits picked.

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> >+#define JS_OWN_SCOPE(cx,scope)     ((scope)->title.ownercx == (cx))
> 
> I don't think factoring this code out into a macro makes the code any
> clearer. The name is misleading, since "owning" a scope is likely to be
> interpreted as holding the scope lock.

The name implication is intended. If title.ownercx == cx then cx effectively holds the lock until its request finishes. But I am open to suggestions about a better name.

Comment 4

[Igor Bukanov]

Attachment: implementation v2

The new version addresses the comment above except I have kept JS_OWN_SCOPE.

Comment 5

[Brendan Eich [:brendan]]

"Own" is overloaded. How about CX_OWNS_SCOPE_TITLE to be as literal as possible?

/be

Comment 6

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/720b50c81f57 - landed with JS_OWN_SCOPE -> CX_OWNS_SCOPE_TITLE rename.

Comment 7

[Ian Neal]

Since this patch went on I get a segmentation fault when starting SeaMonkey with multiple profiles on trying to start the second or subsequent times after building. See bug 545044 for details.
The workaround is to select the profile from the command line using the -P commandline switch.
Backing out the patch on this bug fixes the issue.

Comment 8

[Ian Neal]

Sorry for the bug spam, updated the wrong tab/bug number, I meant to do bug 535656

Comment 9

[Igor Bukanov]

The bug went to mozilla-central on Jan 27 09:50:17:

http://hg.mozilla.org/mozilla-central/rev/720b50c81f57