Last Comment Bug 726944 - Remove JSClass::xdrObject and related functionality
: Remove JSClass::xdrObject and related functionality
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 747617 752282
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-14 02:01 PST by Igor Bukanov
Modified: 2012-05-06 03:07 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (37.09 KB, patch)
2012-02-14 02:01 PST, Igor Bukanov
luke: review+
Details | Diff | Review
v2 (109.47 KB, patch)
2012-02-20 14:42 PST, Igor Bukanov
igor: review+
Details | Diff | Review

Description Igor Bukanov 2012-02-14 02:01:00 PST
Created attachment 596947 [details] [diff] [review]
v1

JSClass::xdrObject provide a way to use XDR API to serialize arbitrary classes. However, this is functionality is unused as our script and function serialization implementation serializes everything directly since they know own data structures. Moreover, among the our own classes only RegExp implements the hook, but the implementation just do what is necessary to serialize the source of regexp and does not preserve any custom properties that can be set of regexp objects.

As such those any embedding that wants to use the hooks will have to right own serialization code for all native objects and may as well use some form of hash table to implement the functionality that JSClass::xdrObject can address.

So the attached patch removes the hook and purge all the relevant code. The patch also moves transcoding of script constants to the jsscript.cpp allowing to remove unused transcoding of magic values.
Comment 1 Luke Wagner [:luke] 2012-02-20 11:59:16 PST
Comment on attachment 596947 [details] [diff] [review]
v1

Review of attachment 596947 [details] [diff] [review]:
-----------------------------------------------------------------

Ultrakill!!

::: js/src/jsapi.h
@@ +3361,5 @@
>      JSClassInternal     reserved0;
>      JSCheckAccessOp     checkAccess;
>      JSNative            call;
>      JSNative            construct;
> +    JSClassInternal     reserved2;

We are not even trying to maintain binary compatibility these days so can we just remove the field?

::: js/src/jsscript.cpp
@@ +551,5 @@
> +            d = vp->toDouble();
> +        if (!JS_XDRDouble(xdr, &d))
> +            return false;
> +        if (xdr->mode == JSXDR_DECODE)
> +            vp->set(comp, DoubleValue(d));

Assuming 'vp' points to a script->const value that was just allocated, can't you use init() and avoid the barrier?

::: js/src/vm/RegExpObject.h
@@ +437,5 @@
>  inline RegExpShared *
>  RegExpToShared(JSContext *cx, JSObject &obj);
>  
> +bool
> +XDRScriptRegExpObject(JSXDRState *xdr, HeapPtrObject *objp);

If you are doing this to js_XDRRegExpObject, can you do the same thing for js_XDRStaticBlockObject as well (move into js::, strip js_)?
Comment 2 Igor Bukanov 2012-02-20 14:42:37 PST
Created attachment 598970 [details] [diff] [review]
v2

This is v1 + using init, not the assignment, to initialize script heap values + removal of js_ prefixes + removal of xdrObject field from JSClass. As the latter requires to touch almost all class definitions, I also removed reserved0 and reserved1 fields.
Comment 4 Ed Morley [:emorley] 2012-02-21 03:39:14 PST
Was this epxected?

Regression :( V8 increase 1.4% on MacOSX 10.6 (rev4) Mozilla-Inbound
--------------------------------------------------------------------
    Previous: avg 9.533 stddev 0.068 of 30 runs up to revision a374a3bb0bc2
    New     : avg 9.667 stddev 0.000 of 5 runs since revision e6ffb760d2f0
    Change  : +0.133 (1.4% / z=1.966)
    Graph   : http://mzl.la/Av94jb 

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/l7K7nX6rP78
Comment 5 Igor Bukanov 2012-02-21 06:53:41 PST
(In reply to Ed Morley [:edmorley] from comment #4)
> Was this epxected?
> 
> Regression :( V8 increase 1.4% on MacOSX 10.6 (rev4) Mozilla-Inbound

No, this has not been expected as the patch is about removal of unused code. The part of the patch that can affect V8 benchmark is a removal of JSClass:: fields. Due to caching effects that may result in a slowdown, but I have never seen access to JSClass fields in performance profiles.

On the other hand the difference is less than 2 sigma and should happen randomly in at least one of 20 cases. So perhaps I was unlucky?
Comment 6 Ed Morley [:emorley] 2012-02-22 04:46:35 PST
https://hg.mozilla.org/mozilla-central/rev/e6ffb760d2f0

(In reply to Igor Bukanov from comment #5)
> On the other hand the difference is less than 2 sigma and should happen
> randomly in at least one of 20 cases. So perhaps I was unlucky?

Happy just to call it that for now :-)

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