Last Comment Bug 766447 - TI: Allow tracking of DOM instances in TypeObjects
: TI: Allow tracking of DOM instances in TypeObjects
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla16
Assigned To: Eric Faust [:efaust]
: Jason Orendorff [:jorendorff]
Depends on: 774953
Blocks: 747288
  Show dependency treegraph
Reported: 2012-06-19 21:12 PDT by Eric Faust [:efaust]
Modified: 2012-08-10 14:39 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (11.09 KB, patch)
2012-06-25 15:18 PDT, Eric Faust [:efaust]
bhackett1024: review+
Details | Diff | Splinter Review

Description User image Eric Faust [:efaust] 2012-06-19 21:12:26 PDT
We need to add a flag to TypeObjects to track whether the objects of that type are DOM instances. It will behave similarly to OBJECT_FLAG_NON_TYPED_ARRAY.
Comment 1 User image Eric Faust [:efaust] 2012-06-25 15:18:00 PDT
Created attachment 636505 [details] [diff] [review]

Add DOM understanding to TI and stop marking all DOM prototypes and instances as UNKNOWN.
Comment 2 User image Brian Hackett (:bhackett) 2012-06-28 07:15:24 PDT
Comment on attachment 636505 [details] [diff] [review]

Review of attachment 636505 [details] [diff] [review]:

::: js/src/jsapi.h
@@ +3647,5 @@
>  #define JSCLASS_HAS_PRIVATE             (1<<0)  /* objects have private slot */
>  #define JSCLASS_NEW_ENUMERATE           (1<<1)  /* has JSNewEnumerateOp hook */
>  #define JSCLASS_NEW_RESOLVE             (1<<2)  /* has JSNewResolveOp hook */
>  #define JSCLASS_PRIVATE_IS_NSISUPPORTS  (1<<3)  /* private is (nsISupports *) */
> +#define JSCLASS_IS_DOMJSCLASS           (1<<4)  /* objects are DOM */

This might be better off in jsfriendapi.h

::: js/src/jsinfer.cpp
@@ +1893,5 @@
>      new(object) TypeObject(proto, key == JSProto_Function, unknown);
>      if (!cx->typeInferenceEnabled())
>          object->flags |= OBJECT_FLAG_UNKNOWN_MASK;
> +    else {

'if' needs {} to match the else

@@ +5709,5 @@
>      if (!table.initialized() && !table.init())
>          return NULL;
>      TypeObjectSet::AddPtr p = table.lookupForAdd(this);
>      if (p) {

In the case where the lookup is successful, this needs to make sure to set the OBJECT_FLAG_NON_DOM bit on the object if !isDOM, as it's currently possible to have different objects that are both DOM and non-DOM and have the same type.

::: js/src/jsinfer.h
@@ +292,5 @@
>      /* For a global object, whether flags were set on the RegExpStatics. */
>      OBJECT_FLAG_REGEXP_FLAGS_SET      = 0x00800000,
> +    /* Whether any objects this represents are not DOM objects. */
> +    OBJECT_FLAG_NON_DOM               = 0x01000000,

It would be nice if this flag went after OBJECT_FLAG_NON_TYPED_ARRAY (and the values for the other flags shuffled a bit), as that group is the one this is logically related to.

::: js/src/jsproxy.cpp
@@ +1767,5 @@
>      /* Don't track types of properties of proxies. */
>      MarkTypeObjectUnknownProperties(cx, obj->type());
> +    /* Set the new proxy as having a singleton type. */

Comment 3 User image Ed Morley [:emorley] 2012-07-12 05:24:37 PDT
Unfortunately bug 772303 ( had to be backed out for winxp pgo-only jsreftest failures.

This bug (amongst others) either caused unresolvable (for someone who doesn't work on the JS engine at least) conflicts with the backout of fad7d06d7dd5, or else bugzilla dependencies indicated that one of it's dependants had now been backed out. 

Since there was no one in #developers that could resolve the conflicts, unfortunately this bug has been backed out:
Comment 4 User image Matt Brubeck (:mbrubeck) 2012-07-12 07:44:10 PDT
This bug also caused regressions in Dromaeo (DOM) and Trace Malloc MaxHeap benchmarks.  Was that expected?
Comment 5 User image Eric Faust [:efaust] 2012-07-12 18:25:01 PDT
I think so, if only a tiny bit. We had to change the way TI marked some newly created objects, in particular proxies, that may lead to small increases in allocations. It shouldn't be much of a hit, though, and it moves towards JIT integration of DOM methods and property accesses.
Comment 7 User image Ed Morley [:emorley] 2012-07-13 05:30:13 PDT

Note You need to log in before you can comment on or make changes to this bug.