Last Comment Bug 606029 - Remove JSObject::title
: Remove JSObject::title
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Jason Orendorff [:jorendorff]
:
:
Mentors:
: 551633 588801 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-20 16:31 PDT by Jason Orendorff [:jorendorff]
Modified: 2015-10-08 08:50 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 (28.92 KB, patch)
2010-10-20 16:33 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
WIP 2 (170.84 KB, patch)
2010-10-20 22:16 PDT, Jason Orendorff [:jorendorff]
igor: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2010-10-20 16:31:42 PDT
Doing this, and making JS_LOCK/UNLOCK_OBJ a no-op, wins 1% on SunSpider and 7% on V8. Saves memory, too (4 words per object).
Comment 1 Jason Orendorff [:jorendorff] 2010-10-20 16:33:50 PDT
Created attachment 484881 [details] [diff] [review]
WIP 1

The patch I used to get those numbers.
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2010-10-20 16:54:00 PDT
(In reply to comment #1)
> The patch I used to get those numbers.

Can we get rid of dropProperty as well, now?
Comment 3 Andreas Gal :gal 2010-10-20 16:56:48 PDT
There is still MT code in extensions. There is at least 2 dups for this bug open right now. I would love to get rid of LOCK/UNLOCK, but there is no consensus for doing that without an alternative mechanism. The only alternative on the table are the MT proxies, and not even I like them.
Comment 4 Andreas Gal :gal 2010-10-20 16:58:42 PDT
7% on V8 is massive btw :(
Comment 5 Brian Hackett (:bhackett) 2010-10-20 17:04:54 PDT
Is the 7% on V8 tripping a GC threshold on splay or earley-boyer (do you get the same # of GCs with and without the patch?).  Each GC on splay is 5% of the total V8 runtime (SS shell harness version) on my machine IIRC.
Comment 6 Brendan Eich [:brendan] 2010-10-20 17:35:58 PDT
Let's do MT proxies. Andreas disliking them is not enough of a reason not to do them. How about that bug (bug 566951) block this one?

/be
Comment 7 Brendan Eich [:brendan] 2010-10-20 17:36:54 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > The patch I used to get those numbers.
> 
> Can we get rid of dropProperty as well, now?

We could but the real win is to switch to an ES5-like MOP. That will entail other changes, so we should have a plan and check it twice.

/be
Comment 8 Jason Orendorff [:jorendorff] 2010-10-20 22:16:20 PDT
Created attachment 484969 [details] [diff] [review]
WIP 2

Looking into the reasons for the win now.

** TOTAL **:      1.070x as fast    2215.3ms +/- 0.4%   2069.9ms +/- 0.3%
=========================================================================
  v8:             1.070x as fast    2215.3ms +/- 0.4%   2069.9ms +/- 0.3%
    crypto:       1.007x as fast     266.5ms +/- 0.3%    264.6ms +/- 0.4%
    deltablue:    1.051x as fast     355.1ms +/- 0.9%    337.8ms +/- 0.4%
    earley-boyer: 1.24x as fast      358.8ms +/- 0.4%    290.4ms +/- 0.5%
    raytrace:     1.082x as fast     407.6ms +/- 1.2%    376.6ms +/- 0.5%
    regexp:       1.008x as fast     318.9ms +/- 0.3%    316.4ms +/- 0.2%
    richards:     -                  242.4ms +/- 1.5%    239.0ms +/- 0.4%
    splay:        1.085x as fast     266.0ms +/- 0.4%    245.1ms +/- 1.0%
Comment 9 Jason Orendorff [:jorendorff] 2010-10-20 22:51:04 PDT
The patch makes earley-boyer do one less GC, from 2 to 1.

It doesn't affect the number of GCs done by raytrace or splay. (Each just does 1 during the test. I don't count the GC_LAST_CONTEXT GC, which happens as we shut down the shell, after the timing is done and the result is printed.)
Comment 10 Igor Bukanov 2010-10-21 08:14:12 PDT
(In reply to comment #9)
> The patch makes earley-boyer do one less GC, from 2 to 1.
> 
> It doesn't affect the number of GCs done by raytrace or splay. (Each just does
> 1 during the test. I don't count the GC_LAST_CONTEXT GC, which happens as we
> shut down the shell, after the timing is done and the result is printed.)

Could you re-run the test with adding 4 words of pads instead of the title to the object. It would be interesting to know if the win comes from smaller objects or eliminating OBJ_LOCK/UNLOCK.
Comment 11 Igor Bukanov 2010-10-21 08:24:23 PDT
(In reply to comment #6)
> Let's do MT proxies.

Do you mean patching the object class dynamically to make it MT? That would be rather hazardous as in quite a few places the code assumes that class does not mutates. Fixing just Proxy.fix to work reliably took a lot of efforts.
Comment 12 Brendan Eich [:brendan] 2010-10-21 10:37:07 PDT
(In reply to comment #11)
> (In reply to comment #6)
> > Let's do MT proxies.
> 
> Do you mean patching the object class dynamically to make it MT? That would be
> rather hazardous as in quite a few places the code assumes that class does not
> mutates. Fixing just Proxy.fix to work reliably took a lot of efforts.

I mean fixing bug 566951. We are using proxies and becomes in our security wrappers and we fixed those poor assumptions. We are not going back on that.

/be
Comment 13 Jason Orendorff [:jorendorff] 2010-10-21 11:49:39 PDT
Discussed this with Brendan on IRC. In a sec, I'll post in bug 566951 about the requirements there.

But in short: let's get this patch landed before it bitrots. We're not going to leave 7% on the table; this should go in, and there's no point delaying it.

As brendan points out, we need to make the right defensive moves so that existing extensions and xulrunner apps don't just start crashing or mysteriously misbehaving. But we can do it in any order during the betas.
Comment 14 Jason Orendorff [:jorendorff] 2010-10-21 11:51:42 PDT
Comment on attachment 484969 [details] [diff] [review]
WIP 2

I'll check today to see if 4 words of padding has the same effect. I'm curious too.
Comment 15 Brendan Eich [:brendan] 2010-10-21 12:21:06 PDT
Win won't be nearly 7% once the GC schedule status quo ante is restored, I bet. Gregor and Igor are on that case, though (makes me hopeful we can move Shapes into the GC heap without another round of just-so-story tuning of GC thresholds to keep benchmarks unregressed).

/be
Comment 16 Igor Bukanov 2010-10-21 13:56:19 PDT
(In reply to comment #12)
> I mean fixing bug 566951. We are using proxies and becomes in our security
> wrappers and we fixed those poor assumptions. 

I do not see how, for example, date_setTime , http://hg.mozilla.org/tracemonkey/file/339457364540/js/src/jsdate.cpp#l1646, is fixed to tolerate a class change in ValueToNumber.
Comment 17 Igor Bukanov 2010-10-22 02:48:28 PDT
Comment on attachment 484969 [details] [diff] [review]
WIP 2

>diff --git a/js/src/jslock.h b/js/src/jslock.h
>--- a/js/src/jslock.h
>+++ b/js/src/jslock.h
>@@ -96,19 +96,19 @@ struct JSTitle {
>     union {                             /* union lockful and lock-free state: */
>         jsrefcount  count;              /* lock entry count for reentrancy */
>         JSTitle     *link;              /* next link in rt->titleSharingTodo */
>     } u;
> };

The patch should remove JSTitle and related code as it removes obj_lock/obj_unlock, the only users of the functionality. HG history will preserve it if we would need it for MT objected.

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>@@ -1389,22 +1386,17 @@ js_HasOwnPropertyHelper(JSContext *cx, L
>+    vp->setBoolean(prop != NULL);

Nit: use vp->setBoolean(!!prop)

>diff --git a/js/src/methodjit/StubCalls.cpp b/js/src/methodjit/StubCalls.cpp

>@@ -2781,18 +2759,14 @@ stubs::In(VMFrame &f)
>+    return prop != NULL;

Nit: use !!prop

r+ with the above fixed.
Comment 18 Paul Wright 2010-10-22 19:24:50 PDT
The win doesn't show on AWFY :(
Comment 19 Andreas Gal :gal 2010-10-22 19:27:03 PDT
awfy runs single-thread shell builds without title in objects, not multi-threaded browser builds.
Comment 20 Paul Wright 2010-10-22 19:35:34 PDT
I understand.  Thank you for clarifying this.
FWIW, this may be (another) good reason to modify the shell (or, even better, the testing methodology AWFY is based upon) to be more representative of the actual browser product, since nobody is going to end up running a shell when all is said and done.

...end of bug spam...
Comment 21 Brendan Eich [:brendan] 2010-10-23 08:36:58 PDT
Paul: the goal is to make browser as fast as shell, not lumber shell with old browser overhead.

The JS_THREADSAFE costs are going away, but this bug may not be enough. The GC schedule can differ between browser and shell for less obvious reasons that the object size being bigger in browser due to JS_THREADSAFE.

/be
Comment 22 Igor Bukanov 2010-10-25 07:16:06 PDT
Jason: this has been landed as http://hg.mozilla.org/tracemonkey/rev/8901fae3833a and  http://hg.mozilla.org/tracemonkey/rev/60bdafdffdb9. Any reason not to add fixed-in-tracemonkey?
Comment 23 Jason Orendorff [:jorendorff] 2010-10-25 09:46:01 PDT
Right, thanks.
Comment 25 Brendan Eich [:brendan] 2010-10-28 14:46:32 PDT
Bug 604756 is the crash-spike I prophesied about on IRC...

/be
Comment 26 Andreas Gal :gal 2010-10-28 14:49:28 PDT
This should not have landed with an exception is dispatch that rejects cross thread closures. I suggested that in irc :-/ I will file a bug to add the exception so we at least get a handle on the problem and know where it happens.
Comment 27 Brendan Eich [:brendan] 2010-10-28 23:12:00 PDT
(In reply to comment #26)
> This should not have landed with an exception is dispatch that rejects cross
> thread closures. I suggested that in irc :-/ I will file a bug

Bug 608142.

/be
Comment 28 Jason Orendorff [:jorendorff] 2010-10-29 14:41:35 PDT
(In reply to comment #26)
> This should not have landed with an exception is dispatch that rejects cross
> thread closures. I suggested that in irc :-/ I will file a bug to add the
> exception so we at least get a handle on the problem and know where it happens.

You are right. Mea culpa.
Comment 29 André Bargull 2015-09-21 10:08:42 PDT
*** Bug 551633 has been marked as a duplicate of this bug. ***
Comment 30 André Bargull 2015-10-08 08:50:57 PDT
*** Bug 588801 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.