726944 - Remove JSClass::xdrObject and related functionality

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Attachment: 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]]

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]

Attachment: 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 3

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ffb760d2f0

Comment 4

[Ed Morley [:emorley]]

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]

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

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 :-)