Closed Bug 726944 Opened 8 years ago Closed 8 years ago

Remove JSClass::xdrObject and related functionality

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file, 1 obsolete file)

v2
109.47 KB, patch
igor
: review+
Details | Diff | Splinter Review
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #596947 - Flags: review?(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_)?
Attachment #596947 - Flags: review?(luke) → review+
Attached patch v2Splinter Review
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.
Attachment #596947 - Attachment is obsolete: true
Attachment #598970 - Flags: review+
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
(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?
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 :-)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Depends on: 752282
You need to log in before you can comment on or make changes to this bug.