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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

Attachment: v1

untested work-in-progress

Comment 2

[Igor Bukanov]

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.

Comment 3

[Igor Bukanov]

(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

Comment 4

[Igor Bukanov]

(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.

Comment 5

[Igor Bukanov]

Attachment: v2

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.

Comment 6

[Igor Bukanov]

Attachment: v3

In v2 I forgot to remove no longer used js_LockObjIfShape.

Comment 7

[Igor Bukanov]

Attachment: v4

More deadwood removal. This time it is JS_LOCK_OBJ_IF_SHAPE and JS_LOCK_OBJ_VOID defines for !JS_THREADSAFE.

Comment 8

[Jason Orendorff [: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.

Comment 9

[Igor Bukanov]

(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.

Comment 10

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/24c332b5276e - landed with comment 8 addressed.

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/24c332b5276e