Last Comment Bug 690133 - Remove JSObject::clasp
: Remove JSObject::clasp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 690943
Blocks: ObjectShrink
  Show dependency treegraph
 
Reported: 2011-09-28 15:43 PDT by Brian Hackett (:bhackett)
Modified: 2011-12-07 16:45 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (77.48 KB, patch)
2011-09-29 08:32 PDT, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Splinter Review
remove background sweep calls to clasp->finalize (23.50 KB, patch)
2011-09-29 20:56 PDT, Brian Hackett (:bhackett)
wmccloskey: review+
Details | Diff | Splinter Review
move inline methods to *inlines.h (51.31 KB, patch)
2011-10-04 18:43 PDT, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-09-28 15:43:03 PDT
This is straightforward to do on top of bug 684505; obj->clasp is already identical to obj->lastProp->base->clasp.
Comment 1 Brian Hackett (:bhackett) 2011-09-29 08:32:15 PDT
Created attachment 563418 [details] [diff] [review]
patch

Note: with bug 684505 a shape guard also guards on class (there is no longer a shared non-native shape, instead one EmptyShape/BaseShape per non-native class in a compartment), so the cost of class guards in e.g. PIC'ed element accesses is unchanged.

https://hg.mozilla.org/projects/jaegermonkey/rev/55a63871f966
Comment 2 Brian Hackett (:bhackett) 2011-09-29 20:56:41 PDT
Created attachment 563645 [details] [diff] [review]
remove background sweep calls to clasp->finalize

Followup fix to work with background finalization.  JSObject::finalize could access the object's clasp->finalize, which now requires the shape to be intact.  This is fine when objects are finalized during GC sweep (shapes are finalized after objects), but when doing background finalization the object's shape is no longer usable.  This patch fixes things so that background finalization does not call clasp->finalize, so objects with finalize hooks cannot be finalized in the background.  This only happened for function and iterator objects.  Iterators are very rare so it's fine to finalize them during sweep.  The function class finalizer only does stuff if the function is a flat closure, and flat closures are due to be removed by bug 687185, which would let functions be finalized in the background again (if that bug doesn't pan out, flat closures should store their data in the function's slots anyways, which would also avoid the need for fun_finalize).

https://hg.mozilla.org/projects/jaegermonkey/rev/c9be55115ad8
Comment 3 Bill McCloskey (:billm) 2011-09-30 17:36:39 PDT
Comment on attachment 563645 [details] [diff] [review]
remove background sweep calls to clasp->finalize

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

::: js/src/jsobjinlines.h
@@ +168,5 @@
>  
> +    if (!background) {
> +        /*
> +         * Finalize obj first, in case it needs map and slots. Objects with
> +         * finalize hooks are not called finalized in the background, as the

"are not called finalized" -> "are not finalized"
Comment 4 Luke Wagner [:luke] 2011-10-03 20:55:56 PDT
Comment on attachment 563418 [details] [diff] [review]
patch

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

::: js/src/jsobjinlines.h
@@ +1611,5 @@
>      return obj->isDenseArray() ? obj->getProto() : obj;
>  }
>  
> +inline js::Class *
> +JSObject::getClass() const {

newline for {, here and below.
Comment 5 Brian Hackett (:bhackett) 2011-10-04 18:43:04 PDT
Created attachment 564738 [details] [diff] [review]
move inline methods to *inlines.h

Move inline methods depending on getClass() and related methods into *inlines.h files.  Otherwise can get weird link errors if someone includes jsobj.h but not jsobjinlines.h.
Comment 6 David Mandelin [:dmandelin] 2011-10-10 17:19:47 PDT
Setting owner according to patch author.

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