Closed Bug 540805 Opened 10 years ago Closed 10 years ago

move JSScope::nrefs to JSEmptyScope

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch implementation v1 (obsolete) — Splinter Review
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
Attachment #423258 - Flags: review?(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.
Attachment #423258 - Flags: review?(jorendorff) → review+
(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.
The new version addresses the comment above except I have kept JS_OWN_SCOPE.
Attachment #423258 - Attachment is obsolete: true
Attachment #423597 - Flags: review+
"Own" is overloaded. How about CX_OWNS_SCOPE_TITLE to be as literal as possible?

/be
https://hg.mozilla.org/tracemonkey/rev/720b50c81f57 - landed with JS_OWN_SCOPE -> CX_OWNS_SCOPE_TITLE rename.
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.
Blocks: 545044
Sorry for the bug spam, updated the wrong tab/bug number, I meant to do bug 535656
No longer blocks: 545044
Whiteboard: fixed-in-tracemonkey
The bug went to mozilla-central on Jan 27 09:50:17:

http://hg.mozilla.org/mozilla-central/rev/720b50c81f57
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 566145
You need to log in before you can comment on or make changes to this bug.