Michael Wu [:mwu] Assignee
	Description • 14 years ago

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

	Michael Wu [:mwu] Assignee
	Comment 1 • 14 years ago

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)

	Michael Wu [:mwu] Assignee
	Comment 2 • 14 years ago

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

	Michael Wu [:mwu] Assignee
	Comment 4 • 14 years ago

Logic improved a bit, fname renamed to filename.

	Michael Wu [:mwu] Assignee
	Comment 5 • 14 years ago

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..

	Igor Bukanov
	Comment 6 • 14 years ago

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.

	Michael Wu [:mwu] Assignee
	Comment 9 • 14 years ago

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

	Igor Bukanov
	Comment 10 • 14 years ago

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.

	Igor Bukanov
	Comment 11 • 14 years ago

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.

	Michael Wu [:mwu] Assignee
	Comment 12 • 14 years ago

Comments addressed.

	Michael Wu [:mwu] Assignee
	Comment 13 • 14 years ago

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.

	Igor Bukanov
	Comment 14 • 14 years ago

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.

	Michael Wu [:mwu] Assignee
	Comment 15 • 14 years ago

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

	Michael Wu [:mwu] Assignee
	Comment 16 • 13 years ago

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