Optimizing the enumeration state allocation

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({perf})

Trunk
Points:
---
Bug Flags:
wanted1.9.1 +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
+++ 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.
(Assignee)

Updated

10 years ago
Blocks: 417131
No longer depends on: 417131
(Assignee)

Comment 1

10 years ago
Created attachment 328235 [details] [diff] [review]
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.
Attachment #328235 - Flags: review?(brendan)
(Assignee)

Comment 2

10 years ago
Created attachment 328236 [details]
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.
(Assignee)

Comment 3

10 years ago
Comment on attachment 328235 [details] [diff] [review]
v1

Patch with readability improvments is comming.
Attachment #328235 - Attachment is obsolete: true
Attachment #328235 - Flags: review?(brendan)
(Assignee)

Comment 4

10 years ago
Created attachment 328240 [details] [diff] [review]
v2

The new version of the patch uses do-while(0) loop to avoid too many nested ifs.
Attachment #328240 - Flags: review?(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
Attachment #328240 - Flags: review?(brendan) → review+
(Assignee)

Comment 6

10 years ago
Created attachment 328262 [details] [diff] [review]
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
-
Attachment #328240 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #328262 - Flags: review+
(Assignee)

Comment 7

10 years ago
pushed: http://hg.mozilla.org/mozilla-central/index.cgi/rev/65d83c60f33a
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: wanted1.9.1+

Updated

9 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.