Closed Bug 538463 Opened 11 years ago Closed 11 years ago

cache only single-threaded objects to avoid JS_UNLOCK_OBJ overhead in js_Interpret

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

v4
35.43 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
Currently all fast code paths in js_Interpret dealing with property cache calls JS_UNLOCK_OBJ. That means that all these paths contain a check like OBJ_SCOPE(obj)->title.ownercx == cx to call js_UnlockObj on mismatch. This overhead can be eliminated if only single-threaded object would be cached. That is, the idea is to cache only objects if OBJ_SCOPE(obj)->title.ownercx == cx which would eliminate the need in the unlock checks.
Attached patch v1 (obsolete) — Splinter Review
untested work-in-progress
The property cache queries in js_Interpret can avoid the overhead of both JS_LOCK_OBJ and JS_UNLOCK_OBJ if ClaimTitle would regenerate the scope shape whenever the object would become shared or claimed by another thread. In addition js_FillPropertyCache would need to cache only objects that belong to the current thread or read-only. 

To implement that I need to know how to change the shape of the scope to invalidate any property cache entries and trace code guards.

brendan, jorendorff - any idea how to do it? I am not sure what to do if the scope does not own the shape. In particular, it seems that I cannot just call JSScope::updateShape. That does nothing if the scope does not have own shape so the scope would be still cached.
(In reply to comment #2)
> The property cache queries in js_Interpret can avoid the overhead of both
> JS_LOCK_OBJ and JS_UNLOCK_OBJ if ClaimTitle would regenerate the scope shape
> whenever the object would become shared or claimed by another thread.

That does not work. brendan has pointed out on irc that for scopes that shares the shape with lastProp->shape there could be N >> 1 scopes with the same shape. To update the shape ClaimTitle would need to find all such scopes and synchronize their shape with the lastProp->shape update. That is not possible.


 After discussion with brendan on irc I realized that for scopes that do not own the shape
(In reply to comment #3)
>  After discussion with brendan on irc I realized that for scopes that do not
> own the shape

Please ignore that sentence - that was an early draft.
Attached patch v2 (obsolete) — Splinter Review
The new patch is a fixed version of v1 and does not try to eliminate title.ownercx == cx check and focuses just on eliminating lock/unlock calls. 

It shows 1.5% improvement in sunspider with tracing disabled with no differences with tracing on. This is not surprising as the patch changes pretty much only js_Interpret(). For the v8 benchmark the speedup is 2-3% with tracing off and 1.5% with tracing on. This is for 1.6GHz ATOM N270 CPU. For 2.6 Core2-based Xenon the results are similar.
Attachment #420715 - Attachment is obsolete: true
Attachment #423800 - Flags: review?(jorendorff)
Attached file v3 (obsolete) —
In v2 I forgot to remove no longer used js_LockObjIfShape.
Attachment #423800 - Attachment is obsolete: true
Attachment #423823 - Flags: review?(jorendorff)
Attachment #423800 - Flags: review?(jorendorff)
Attached patch v4Splinter Review
More deadwood removal. This time it is JS_LOCK_OBJ_IF_SHAPE and JS_LOCK_OBJ_VOID defines for !JS_THREADSAFE.
Attachment #423823 - Attachment is obsolete: true
Attachment #423831 - Flags: review?(jorendorff)
Attachment #423823 - Flags: review?(jorendorff)
Comment on attachment 423831 [details] [diff] [review]
v4

Nice patch.

You missed a call to JS_UNLOCK_OBJ in TraceRecorder::test_property_cache.

In case JSOP_NEWINIT:
>         if (regs.pc[JSOP_NEWINIT_LENGTH] != JSOP_ENDINIT) {
>             JS_LOCK_OBJ(cx, obj);
>             JSScope *scope = js_GetMutableScope(cx, obj);
>             if (!scope) {
>                 JS_UNLOCK_OBJ(cx, obj);
>                 goto error;
>             }
>-            JS_UNLOCK_SCOPE(cx, scope);
>+
>+            /*
>+             * A new object debugger hook may add properties and share the
>+             * newly created object with other threads. Thus we cannot assume
>+             * that js_GetMutableScope creates a mutable scope here owned by
>+             * cx and skip JS_UNLOCK_OBJ.
>+             */
>+            JS_UNLOCK_OBJ(cx, obj);

I agree with the reasoning, but is it necessary to change JS_UNLOCK_SCOPE to
JS_UNLOCK_OBJ?

In jsscope.h, struct JSScope:
>+    void threadShapeChange(JSContext *cx);

Left over from earlier hacking? This isn't defined or used anywhere.

In jsinterpinlines.h:
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *

Make the Initial Developer Mozilla Corporation.
Update the copyright year.
Fill in the contributor.

r=me with nits picked.
Attachment #423831 - Flags: review?(jorendorff) → review+
(In reply to comment #8)
> You missed a call to JS_UNLOCK_OBJ in TraceRecorder::test_property_cache.

That was a good catch.

> In case JSOP_NEWINIT:
> >+            JS_UNLOCK_OBJ(cx, obj);
...
> >+    void threadShapeChange(JSContext *cx);

That UNLOCK_SCOPE->UNLOCK_OBJ together with threadShapeChange were indeed leftover from earlier versions.
https://hg.mozilla.org/tracemonkey/rev/24c332b5276e - landed with comment 8 addressed.
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/24c332b5276e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.