632253 - Avoid redundant serialization of script filename

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Michael Wu [:mwu]]

We can use utf-8 for strings and avoid repetition of filenames.

Comment 1

[Michael Wu [:mwu]]

Attachment: WIP: Use utf8, avoid repeating filenames

This patch reduces the size of xdr serialized scripts by 25% or more in many cases. It requires that scripts use valid utf16 to be serialized, and I still need to figure out how to force utf8 when converting in certain cases. Unfortunately, the actual entropy of the output isn't significantly changed, so the size while compressed isn't much smaller. (around 5% IIRC)

Comment 2

[Michael Wu [:mwu]]

Reducing the scope of this bug to just avoiding the repetition of filenames.

Comment 3

[Michael Wu [:mwu]]

Attachment: Avoid repeating filenames

Comment 4

[Michael Wu [:mwu]]

Attachment: Avoid repeating filenames, v2

Logic improved a bit, fname renamed to filename.

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 513006 [details] [diff] [review]
Avoid repeating filenames, v2

Igor, can you review this patch or should someone else do it? No rush, at any rate.

IIRC this patch saved about 7% or so in the serialized version of XPIProvider.jsm.

This patch adds a bit of a regression in that we can't serialize jsscripts that have filenames with jsscripts within that don't have filenames. I'm not sure if that actually happens in practice, as most jsscripts inside a jsscript all seem to have the same filenames..

Comment 6

[Igor Bukanov]

Comment on attachment 513006 [details] [diff] [review]
Avoid repeating filenames, v2

>@@ -606,9 +607,39 @@ js_XDRScript(JSXDRState *xdr, JSScript *
>     if (!ok)
>         goto error;
> 
>-    if (!JS_XDRBytes(xdr, (char *)notes, nsrcnotes * sizeof(jssrcnote)) ||
>-        !JS_XDRCStringOrNull(xdr, (char **)&script->filename) ||
>-        !JS_XDRUint32(xdr, &lineno) ||
>+    if (!JS_XDRBytes(xdr, (char *)notes, nsrcnotes * sizeof(jssrcnote))) {
>+        goto error;
>+    }

Style nit: drop {} goto - SM style always omits {} around single statement that fits one line.

>+    if (xdr->mode == JSXDR_ENCODE) {
>+        // this can't happen, right?
>+        if (xdr->filename && !script->filename)
>+            goto error;

You are right in the observation, but you should add an assert, not an unreported failure. 

But I think the right approach here would be to move the serialization of the magic constant code from the beginning of js_XDRScript into JS_XDRScript, the public API entry, so it can also be done only once and then do the script name serialization there. Then the code can avoid any strcmp and related logic in js_XDRScript that the patch adds.

Comment 7

[Michael Wu [:mwu]]

Attachment: Move magic check to JS_XDRScript

Comment 8

[Michael Wu [:mwu]]

Attachment: Move filename check to JS_XDRScript

Comment 9

[Michael Wu [:mwu]]

Attachment: Move filename check to JS_XDRScript, v2

Just realized the assertion can be simplified since the filename strings are basically atoms.

Comment 10

[Igor Bukanov]

Comment on attachment 514851 [details] [diff] [review]
Move magic check to JS_XDRScript

> JS_XDRScript(JSXDRState *xdr, JSScript **scriptp)
> {
>-    if (!js_XDRScript(xdr, scriptp, NULL))
>+    uint32 magic;
>+    if (xdr->mode == JSXDR_ENCODE)
>+        magic = JSXDR_MAGIC_SCRIPT_CURRENT;
>+    if (!JS_XDRUint32(xdr, &magic))
>+        return JS_FALSE;
>+    if (magic != JSXDR_MAGIC_SCRIPT_CURRENT) {
>+        /* We do not provide binary compatibility with older scripts. */
>+        JS_ReportErrorNumber(xdr->cx, js_GetErrorMessage, NULL, JSMSG_BAD_SCRIPT_MAGIC);
>+        return JS_FALSE;
>+    }
>+
>+    if (!js_XDRScript(xdr, scriptp))
>         return JS_FALSE;

Nit: change all JS_FALSE|JS_TRUE into false|true.

Comment 11

[Igor Bukanov]

Comment on attachment 514856 [details] [diff] [review]
Move filename check to JS_XDRScript, v2

>diff --git a/js/src/jsxdrapi.cpp b/js/src/jsxdrapi.cpp
>--- a/js/src/jsxdrapi.cpp
>+++ b/js/src/jsxdrapi.cpp
>@@ -679,6 +679,21 @@ JS_XDRScript(JSXDRState *xdr, JSScript *
>         return JS_FALSE;
>     }
> 
>+    if (xdr->mode == JSXDR_ENCODE)
>+        xdr->filename = (*scriptp)->filename;

This should be DEBUG-only as during encoding  xdr->filename is used only to assert.

>+    if (!JS_XDRCStringOrNull(xdr, (char **)&xdr->filename))
>+        return JS_FALSE;

Nit: replace JS_FALSE|JS_TRUE with false|true in the new code.

>+    if (xdr->mode == JSXDR_DECODE) {
>+        const char *filename = xdr->filename;
>+        if (filename) {
>+            filename = js_SaveScriptFilename(xdr->cx, filename);
>+            xdr->cx->free((void *) xdr->filename);
>+            if (!filename)
>+                return JS_FALSE;

This will leave a pointer to freed filename stored in xdr->filename. This is very fragile. To emphasis that xdr->filename is temporary storage to pass extra information to js_XDRScript during decoding I suggest to assert at the start of JS_XDRScript that xdr->filename is null and make sure that it is always cleared on exit. 

r+ with that fixed.

Comment 12

[Michael Wu [:mwu]]

Attachment: Move magic check to JS_XDRScript, v2

Comments addressed.

Comment 13

[Michael Wu [:mwu]]

Attachment: Move filename check to JS_XDRScript, v3

All comments addressed except for the one about making the xdr->filename setting debug only. xdr->filename is used immediately after by JS_XDRCStringOrNull. Using a local var there instead of xdr->filename doesn't seem to improve the code much, though it can be done. (*scriptp)->filename can't be used since *scriptp can be null when decoding.

Comment 14

[Igor Bukanov]

Comment on attachment 514957 [details] [diff] [review]
Move filename check to JS_XDRScript, v3

>@@ -679,7 +682,22 @@ JS_XDRScript(JSXDRState *xdr, JSScript *
>         return false;
>     }
> 
>-    if (!js_XDRScript(xdr, scriptp))
>+    if (xdr->mode == JSXDR_ENCODE)
>+        xdr->filename = (*scriptp)->filename;
>+    if (!JS_XDRCStringOrNull(xdr, (char **) &xdr->filename))
>+        return false;
>+    if (xdr->mode == JSXDR_DECODE && xdr->filename) {
>+        const char *filename = xdr->filename;
>+        filename = js_SaveScriptFilename(xdr->cx, filename);
>+        xdr->cx->free((void *) xdr->filename);
>+        xdr->filename = filename;
>+        if (!filename)
>+            return false;
>+    }

A local variable like char * filename would allow to have one cast less here, but this is sort of "better is the enemy of good". Besides, we should replace filename with plain JSString * which would allow to simplify the above code to much greater extent that having some local variable. I will file a followup about it.

Comment 15

[Michael Wu [:mwu]]

Had to unbitrot the patches but they're still mostly the same, fortunately.

http://hg.mozilla.org/tracemonkey/rev/228241315c2a
http://hg.mozilla.org/tracemonkey/rev/fcb690f3ec66

Comment 16

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/228241315c2a
http://hg.mozilla.org/mozilla-central/rev/fcb690f3ec66