Closed Bug 632253 Opened 9 years ago Closed 9 years ago

Avoid redundant serialization of script filename

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mwu, Assigned: mwu)

References

(Blocks 2 open bugs)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 6 obsolete files)

We can use utf-8 for strings and avoid repetition of 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)
Assignee: general → mwu
Reducing the scope of this bug to just avoiding the repetition of filenames.
Summary: Optimize serialization of xdr strings → Avoid redundant serialization of script filename
Attached patch Avoid repeating filenames (obsolete) — Splinter Review
Attachment #510471 - Attachment is obsolete: true
Blocks: 104170
Attached patch Avoid repeating filenames, v2 (obsolete) — Splinter Review
Logic improved a bit, fname renamed to filename.
Attachment #511574 - Attachment is obsolete: true
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..
Attachment #513006 - Flags: review?(igor)
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.
Attachment #513006 - Flags: review?(igor)
Attached patch Move magic check to JS_XDRScript (obsolete) — Splinter Review
Attachment #513006 - Attachment is obsolete: true
Attachment #514851 - Flags: review?
Attachment #514851 - Flags: review? → review?(igor)
Attachment #514853 - Flags: review?(igor)
Just realized the assertion can be simplified since the filename strings are basically atoms.
Attachment #514853 - Attachment is obsolete: true
Attachment #514856 - Flags: review?(igor)
Attachment #514853 - Flags: review?(igor)
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.
Attachment #514851 - Flags: review?(igor) → review+
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.
Attachment #514856 - Flags: review?(igor) → review+
Comments addressed.
Attachment #514851 - Attachment is obsolete: true
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.
Attachment #514856 - Attachment is obsolete: true
Attachment #514957 - Flags: review?(igor)
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.
Attachment #514957 - Flags: review?(igor) → review+
Blocks: 636711
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
Whiteboard: fixed-in-tracemonkey
Depends on: 643927
http://hg.mozilla.org/mozilla-central/rev/228241315c2a
http://hg.mozilla.org/mozilla-central/rev/fcb690f3ec66
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.