Remove support for non-memory XDR operations

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla14
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
117.43 KB, patch
luke
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #730221 +++

Currently XDR API provides infrastructure to transcode scripts and functions into a generic storage. However, this support is rather costly is it means that most XDR operations including simple one like reading 4 bytes goes via a indirect call. In addition it requires to allocate XDR objects on the heap rather than on the stack.

For this reason I suggest to make XDR memory-only with most methods inlined to transcode straight to a byte array. Another idea is to use template<XDRMode mode> to eliminate runtime checks for XDR encoding/decoding mode.
(Assignee)

Comment 1

5 years ago
Created attachment 608335 [details] [diff] [review]
v1

The patch allows to encode/decode scripts and functions only to/from memory. In addition the XDR implementation uses template<XDRMode> to avoid runtime checks for xdr encoding/decoding modes. See comments at the start of the patch for details.
Assignee: general → igor
Attachment #608335 - Flags: review?(luke)

Comment 2

5 years ago
Comment on attachment 608335 [details] [diff] [review]
v1

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

Wowzers.  Overall, this looks great.  Any chance you could instrument a browser and see how much it saves on total startup time?  (Snappy would probably like to hear about it if it's substantial.)

I have a bit more to review, but I'll post what I have now.

In accordance with the Great Incremental Renaming, how about renaming:
  js/src/jsxdrapi.h ==> js/public/XdrApi.h
  js/src/jsxdr.h    ==> js/src/vm/Xdr.h
and split js/src/jsxdrapi.cpp into
  js/src/vm/Xdr.cpp (for vm/Xdr.h XDRState::* definitions)
  js/src/XdrApi.cpp (for XdrApi.h JS_* definitions)

::: js/src/js.msg
@@ -149,5 @@
> -MSG_DEF(JSMSG_END_OF_DATA,             63, 0, JSEXN_INTERNALERR, "unexpected end of data")
> -MSG_DEF(JSMSG_SEEK_BEYOND_START,       64, 0, JSEXN_INTERNALERR, "illegal seek beyond start")
> -MSG_DEF(JSMSG_SEEK_BEYOND_END,         65, 0, JSEXN_INTERNALERR, "illegal seek beyond end")
> -MSG_DEF(JSMSG_END_SEEK,                66, 0, JSEXN_INTERNALERR, "illegal end-based seek")
> -MSG_DEF(JSMSG_WHITHER_WHENCE,          67, 1, JSEXN_INTERNALERR, "unknown seek whence: {0}")

heh

::: js/src/jsatom.cpp
@@ +666,5 @@
> +#if JS_HAS_XDR
> +
> +template<XDRMode mode>
> +bool
> +XDRAtom(XDRState<mode> *xdr, JSAtom **atomp)

The style we agree on earlier is js::XDRAtom, instead of a surrounding namespace js {}.

@@ +674,5 @@
> +        return xdr->codeString(&str);
> +    }
> +
> +    /*
> +     * Inline JS_XDRString when decoding to avoid JSString allocation

XDRState::codeString

::: js/src/jsscript.cpp
@@ +665,3 @@
>              return false;
> +        if (mode == XDR_DECODE)
> +            script->filename = SaveScriptFilename(cx, filename);

I like what you did with codeCString.  It seems like SaveScriptFilename can still fail and requires a check, though.

@@ +681,5 @@
> +         * compilation errors when instantiating when mode is XDR_ENCODE.
> +         */
> +        XDRDecoder *decoder =
> +            static_cast<XDRDecoder *>(reinterpret_cast<XDRState<XDR_DECODE> *>(xdr));
> +        decoder->initScriptPrincipals(script);

Instead of this monkey business, what about putting initScriptPrincipals on XDRState and then just putting JS_ASSERT(mode == XDR_DECODE).

::: js/src/vm/RegExpObject.cpp
@@ +705,4 @@
>  /* Functions */
>  
>  JSObject *
> +CloneRegExpObject(JSContext *cx, JSObject *obj, JSObject *proto)

wrong direction

@@ +757,2 @@
>  bool
> +XDRScriptRegExpObject(XDRState<mode> *xdr, HeapPtrObject *objp)

Again, js::

::: js/src/vm/ScopeObject.cpp
@@ +935,5 @@
>      JS_ResolveStub,
>      JS_ConvertStub
>  };
>  
> +namespace js {

js::
(Assignee)

Comment 3

5 years ago
(In reply to Luke Wagner [:luke] from comment #2)
> Wowzers.  Overall, this looks great.  Any chance you could instrument a
> browser and see how much it saves on total startup time?

It is not straightforward due to lazy loading of the cache so there is no single entry that loads everything. In addition, loading into an empty page does not load all cache entries.

> In accordance with the Great Incremental Renaming, how about renaming:
>   js/src/jsxdrapi.h ==> js/public/XdrApi.h
>   js/src/jsxdr.h    ==> js/src/vm/Xdr.h
> and split js/src/jsxdrapi.cpp into
>   js/src/vm/Xdr.cpp (for vm/Xdr.h XDRState::* definitions)
>   js/src/XdrApi.cpp (for XdrApi.h JS_* definitions)

With the patch the XDR API set is reduced to 4 entries/ So what about simply moving the API into jsapi.(h|cpp) and keeping just vm/xdr.(h|cpp_

> > +        XDRDecoder *decoder =
> > +            static_cast<XDRDecoder *>(reinterpret_cast<XDRState<XDR_DECODE> *>(xdr));
> > +        decoder->initScriptPrincipals(script);
> 
> Instead of this monkey business, what about putting initScriptPrincipals on
> XDRState and then just putting JS_ASSERT(mode == XDR_DECODE).

The reason I put initScriptPrincipals to the encoder is to emphasize that the principals are decoder-only feature. But I agree that the double cast looks ugly. So I guess I will move the principals and the init method to the XDRState.

Comment 4

5 years ago
(In reply to Igor Bukanov from comment #3)
> It is not straightforward due to lazy loading of the cache so there is no
> single entry that loads everything.

What about simply putting 'rdtsc' at the beginning end of all public XDR APIs?  The overhead of rdtsc should be amortized enough that the sum gives you an accurate idea over total time in XDR before/after.

> With the patch the XDR API set is reduced to 4 entries/ So what about simply
> moving the API into jsapi.(h|cpp) and keeping just vm/xdr.(h|cpp_

Ah, yes, sounds good.
(Assignee)

Comment 5

5 years ago
Created attachment 608639 [details] [diff] [review]
v2

The patch addresses the comments. I also removed JS_HAS_XDR. As CloneScript depends on XDR !JS_HAS_XDR has been broken for quite some time.
Attachment #608335 - Attachment is obsolete: true
Attachment #608335 - Flags: review?(luke)
Attachment #608639 - Flags: review?(luke)

Comment 6

5 years ago
Comment on attachment 608639 [details] [diff] [review]
v2

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

Nicely done

::: js/src/jsapi.cpp
@@ +105,5 @@
>  #include "vm/RegExpObject-inl.h"
>  #include "vm/RegExpStatics-inl.h"
>  #include "vm/Stack-inl.h"
>  #include "vm/String-inl.h"
> +#include "vm/Xdr.h"

This #include should be below vm/StringBuffer.h

Here is the general style:
  https://wiki.mozilla.org/JavaScript:SpiderMonkey:C++_Coding_Style#Includes
There are 4 or so other #includes that need reordering; could you grep the patch for '#include' and fix the rest up?

::: js/src/jsatom.cpp
@@ +681,5 @@
> +#if IS_LITTLE_ENDIAN
> +    /* Directly access the little endian chars in the XDR buffer. */
> +    const jschar *chars = reinterpret_cast<const jschar *>(xdr->buf.read(nchars * sizeof(jschar)));
> +    atom = js_AtomizeChars(cx, chars, nchars);
> +#else

Nice

::: js/src/jsxdrapi.cpp
@@ +89,5 @@
> +        js_ReportOutOfMemory(cx());
> +        return false;
> +    }
> +    if (data != base) {
> +        base = static_cast<uint8_t *>(data);

Seems like the branch isn't really necessary.

::: js/src/vm/Xdr.h
@@ +59,5 @@
> +
> +class XDRBuffer {
> +  public:
> +    XDRBuffer(JSContext *cx)
> +        : context(cx), base(NULL), cursor(NULL), limit(NULL) { }

Two space indent for : instead of four.

@@ +93,5 @@
> +        return ptr;
> +    }
> +
> +    uint8_t *write(size_t n) {
> +        if (n > size_t(limit - cursor)) {

Might be worth storing 'length' directly to avoid subtraction on every write.

@@ +122,5 @@
> +
> +#if defined IS_LITTLE_ENDIAN
> +
> +inline uint32_t
> +SwabXDR32(uint32_t x)

Is Swab standard lingo?  Since there are only a few uses, how about we go long and say NormalizeByteOrder(16|32)?

@@ +188,5 @@
> +            uint8_t *ptr = buf.write(sizeof tmp);
> +            if (!ptr)
> +                return false;
> +            tmp = SwabXDR16(*n);
> +            memcpy(ptr, &tmp, sizeof tmp);

It seems like memcpy would, in the best case, generate the same code as
  *(uint16_t *)ptr = tmp;
and I would prefer to read this.  If there is not another reason, could you turn all the scalar memcpy's into assignments?
Attachment #608639 - Flags: review?(luke) → review+
(Assignee)

Comment 7

5 years ago
(In reply to Luke Wagner [:luke] from comment #6)
> > +            tmp = SwabXDR16(*n);
> > +            memcpy(ptr, &tmp, sizeof tmp);
> 
> It seems like memcpy would, in the best case, generate the same code as
>   *(uint16_t *)ptr = tmp;
> and I would prefer to read this.

The patch removes 4-byte padding that the current XDR code has. As such the reads can be misaligned. For such reads in general one cannot use assigns and memcpy usage is a must. For x86/x64 CPUs that allow misaligned memory access GCC correctly transforms those 4-bytes memcpy back into assignments.

Comment 8

5 years ago
(In reply to Igor Bukanov from comment #7)
Ah, that's the reason.  Thanks for explaining.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f33e1e959036
Went all red, so backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a39ee24bd89
(Assignee)

Comment 11

5 years ago
relanded with a fix for improper inbound import - https://hg.mozilla.org/integration/mozilla-inbound/rev/30798fdc5bad
All red and backed out again.
http://30.media.tumblr.com/tumblr_m11ykqEl0P1rrf1eeo1_500.jpg
(Assignee)

Comment 14

5 years ago
(In reply to Dão Gottwald [:dao] from comment #12)
> All red and backed out again.

Sorry about that - I forgot to commit the merge fix before pushing again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6084350c40
(Assignee)

Comment 15

5 years ago
To measure the effect of the patch I measured using rdtsc number of CPU cycles spent between JS_XDRNewMem/JS_XDRDestroy before the patch and between XDRState constructor/destructor after the patch. When browser starts to show 2 tabs, one with gmail, and one with about:memory (this way most of XDR cache is exercised) I have:

Before the the patch I got 47.4 millions of cycle, the patch reduced that to 33.2, which translates into a win of 11.1 millions of cycles or 3.7 ms of start up time on 3.0 GHz desktop.
https://hg.mozilla.org/mozilla-central/rev/6b6084350c40
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.