443746 - Optimizing the enumeration state allocation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #417131 +++

Currently to implement the native property enumerator the code allocates separated JSNativeEnumerator and JSIdArray structures. Moreover, this pair is allocated even when the object has no enumerable properties.

It would be nice to fuse the separated allocations int one and avoid allocating anything for empty enumerators.

Note that the patches starting from bug 417131 comment 29 and later already implement the optimizations as a part of id array caching. But to see the clear effects of caching it is necessary to separate the caching and fusing parts into different patches.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch is based on the patch from bug 417131 comment 47 with cache removed and some extra changes to minimize the code bloat (the patch shrinks the size of optimized js shell executable by 60 bytes).

It shows 3.7% improvement with string-fast SunSpider benchmark on 64 bit Linux sm-valgrind01 box, see the following attachment.

Comment 2

[Igor Bukanov]

Attachment: sunspider results, thread-safe jsshell, 64-bit Linux (sm-valgrind01)

I have used -falign-functions=4096 for all the source files when compiling the shell to get the results. Otherwise the code movements that the patch introduces add too much noise.

Comment 3

[Igor Bukanov]

Comment on attachment 328235 [details] [diff] [review]
v1

Patch with readability improvments is comming.

Comment 4

[Igor Bukanov]

Attachment: v2

The new version of the patch uses do-while(0) loop to avoid too many nested ifs.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 328240 [details] [diff] [review]
v2

>+         * We use do-while(0) loop to avoid too much nested ifs. If after the
>+         * loop ne is null, it indicates an empty enumerator.

Suggest "We use a do-while(0) loop" or just "Use a do-while(0) loop".

Also "If ne is null after the loop" adds clarity, both in terms of conventional phrasing in English (preposition after clause) and because ne is a code citation, but could be misread as a typo more easily after loop.

>+++ b/js/src/jsobj.h	Sun Jul 06 04:38:43 2008 +0200
>@@ -604,21 +604,18 @@ extern JSBool
> extern JSBool
> js_DeleteProperty(JSContext *cx, JSObject *obj, jsid id, jsval *rval);
> 
> extern JSBool
> js_DefaultValue(JSContext *cx, JSObject *obj, JSType hint, jsval *vp);
> 
> extern JSIdArray *
> js_NewIdArray(JSContext *cx, jsint length);
> 
>-/*
>- * Unlike realloc(3), this function frees ida on failure.
>- */
> extern JSIdArray *
> js_SetIdArrayLength(JSContext *cx, JSIdArray *ida, jsint length);

This prototype should go away too, now that SetIdArrayLength is static -- or I am misreading something.

r=me with nits and leftovers picked.

/be

Comment 6

[Igor Bukanov]

Attachment: v2b

The new version of the patch fixes nits, here is a delta against v2:

diff -r 07fb16c06195 js/src/jsobj.cpp
--- a/js/src/jsobj.cpp  Sat Jul 05 19:37:47 2008 -0700
+++ b/js/src/jsobj.cpp  Sun Jul 06 11:15:54 2008 -0700
@@ -4153,5 +4153,5 @@ js_Enumerate(JSContext *cx, JSObject *ob
     JSEnumerateOp enumerate;
+    JSNativeEnumerator *ne;
     uint32 length;
     JSScope *scope;
-    JSNativeEnumerator *ne;
     JSScopeProperty *sprop;
@@ -4176,4 +4176,4 @@ js_Enumerate(JSContext *cx, JSObject *ob
          *
-         * We use do-while(0) loop to avoid too much nested ifs. If after the
-         * loop ne is null, it indicates an empty enumerator.
+         * Use a do-while(0) loop to avoid too many nested ifs. If ne is null
+         * after the loop, it indicates an empty enumerator.
          */
diff -r 07fb16c06195 js/src/jsobj.h
--- a/js/src/jsobj.h    Sat Jul 05 19:37:47 2008 -0700
+++ b/js/src/jsobj.h    Sun Jul 06 11:15:54 2008 -0700
@@ -609,8 +609,2 @@ js_DefaultValue(JSContext *cx, JSObject

-extern JSIdArray *
-js_NewIdArray(JSContext *cx, jsint length);
-
-extern JSIdArray *
-js_SetIdArrayLength(JSContext *cx, JSIdArray *ida, jsint length);
-
 extern JSBool
-

Comment 7

[Igor Bukanov]

pushed: http://hg.mozilla.org/mozilla-central/index.cgi/rev/65d83c60f33a