Closed Bug 627016 Opened 9 years ago Closed 9 years ago

remove JSProperty out param from DefineNativeProperty.

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

Currently DefineNativeProperty returns boolean and takes JSProperty ** as a conditional out param. We can remove the parameter and instead return the added shape or null on errors. This would also remove a few JSProperty/Scope casts.
Attached patch v1 (obsolete) — Splinter Review
The patch removes JSProperty out param from DefineNativeProperty as suggested in the comment 0. Another related cleanup is not returning the prototype chain depth from LookupNativeProperty. That depth is only used when filling the property cache and it is simpler to calculate it there instead of verifying the passed parameter.
Attachment #520053 - Flags: review?(jorendorff)
Attached patch v2 (obsolete) — Splinter Review
The new patch fixes comments and replaces "return -1" by "return false" in LookupNativePropertyWithFlagsInline that I missed in v1.
Attachment #520053 - Attachment is obsolete: true
Attachment #520617 - Flags: review?(jorendorff)
Attachment #520053 - Flags: review?(jorendorff)
Attached patch v3 (obsolete) — Splinter Review
v2 got rotten and has to be updated
Attachment #520617 - Attachment is obsolete: true
Attachment #522182 - Flags: review?(jorendorff)
Attachment #520617 - Flags: review?(jorendorff)
Jason: review ping
I'm sorry I dropped this. If you update it again, I promise a review in 48 hours. I'd love to get this patch in.
Comment on attachment 522182 [details] [diff] [review]
v3

Clearing the review flag for now.
Attachment #522182 - Flags: review?(jorendorff)
Attached patch v4Splinter Review
Here is unrotten patch
Attachment #522182 - Attachment is obsolete: true
Attachment #531720 - Flags: review?(jorendorff)
Comment on attachment 531720 [details] [diff] [review]
v4

Review of attachment 531720 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the changes suggested below.

In jsobj.cpp, DefineNativeProperty:
>+  error:
>+    /* TRACE_1 jumps here on error. */
>+    return false;

return NULL.

In js_FindPropertyHelper:
>                 if (clasp == &js_BlockClass) {
>                     /*
>                      * A block instance on the scope chain is immutable and it
>                      * shares its shapes with its compile-time prototype.
>                      */
>                     JS_ASSERT(pobj == obj);
>                     JS_ASSERT(pobj->isClonedBlock());
>-                    JS_ASSERT(protoIndex == 0);
>                 } else {
>                     /* Call and DeclEnvClass objects have no prototypes. */
>+                    JS_ASSERT(pobj == obj);
>                     JS_ASSERT(!obj->getProto());
>-                    JS_ASSERT(protoIndex == 0);
>                 }

Both arms of the if-statement assert that pobj == obj. Why not just assert it
once, outside the if?

In jspropertycache.cpp, PropertyCache::fill:
>      * Check for overdeep scope and prototype chain. Because resolve, getter,
>      * and setter hooks can change the prototype chain using JS_SetPrototype
>+     * after LookupPropertyWithFlags has returned we calculate the protoIndex
>+     * here and not in LookupPropertyWithFlags.

Micro-nit: please consider a comma after "returned".

>+    uintN protoIndex = 0;
>+    do {
>+        if (pobj == obj)
>+            break;
>+        
>         JSObject *tmp = obj;
>+        if (scopeIndex != 0) {
>+            if (scopeIndex > PCVCAP_SCOPEMASK) {
>+                PCMETER(longchains++);
>+                return JS_NO_PROP_CACHE_FILL;
>+            }
>+            for (uintN i = 0; i != scopeIndex; i++)
>+                tmp = tmp->getParent();
>+            if (tmp == pobj)
>+                break;
>+        }
> 
>         protoIndex = 1;
>         for (;;) {
>             tmp = tmp->getProto();

It looks like you've hand-tuned this code to avoid a few conditional branches.

Unless this makes us observably faster (which seems very unlikely given that
we're called after a full LookupProperty), I much prefer the old code. It's a
lot easier to look at it and see what's going on. Suggestion:

+    uintN protoIndex = 0;
-    if (protoIndex != 0) {
         JSObject *tmp = obj;
-
         for (uintN i = 0; i != scopeIndex; i++)
             tmp = tmp->getParent();
         JS_ASSERT(tmp != pobj);

-        protoIndex = 1;
-        for (;;) {
+        while (tmp != pobj) {
             tmp = tmp->getProto();

             /*
              * We cannot cache properties coming from native objects behind
              * non-native ones on the prototype chain. The non-natives can
              * mutate in arbitrary way without changing any shapes.
              */
             if (!tmp || !tmp->isNative()) {
                 PCMETER(noprotos++);
                 return JS_NO_PROP_CACHE_FILL;
             }
-            if (tmp == pobj)
-                break;
             ++protoIndex;
         }
-    }

In jsprvtd.h:
> /* The alignment required of objects stored in GC arenas. */
>-static const uintN JS_GCTHING_ALIGN = 8;
> static const uintN JS_GCTHING_ZEROBITS = 3;
>+static const size_t JS_GCTHING_ALIGN = 8;

This doesn't seem related. Move it to the relevant patch? If there isn't one,
then r=me to check this in as a separate changeset from the main changes here.
Attachment #531720 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/tracemonkey/rev/787c58add0d2 - landed with the comments addressed and without unrelated jsprvtd.h changes.
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/787c58add0d2
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.