Last Comment Bug 652931 - Proper handling of large mark stack insertion
: Proper handling of large mark stack insertion
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
: 656761 (view as bug list)
Depends on:
Blocks: 655562
  Show dependency treegraph
 
Reported: 2011-04-26 12:54 PDT by Gregor Wagner [:gwagner]
Modified: 2011-07-09 07:39 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.69 KB, patch)
2011-04-26 12:55 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (1.88 KB, patch)
2011-04-28 08:56 PDT, Gregor Wagner [:gwagner]
wmccloskey: review+
Details | Diff | Review

Description Gregor Wagner [:gwagner] 2011-04-26 12:54:33 PDT
I noticed that for the FOTN the large markstack gets pretty full.
Comment 1 Gregor Wagner [:gwagner] 2011-04-26 12:55:49 PDT
Created attachment 528404 [details] [diff] [review]
patch
Comment 2 Bill McCloskey (:billm) 2011-04-26 13:35:12 PDT
Wait, this seems like a serious bug to me. Is there any guarantee that the assertion will succeed?

If push returns false, we should probably just do the normal thing and not use the large object path.

Thanks for finding this!
Comment 3 Gregor Wagner [:gwagner] 2011-04-26 16:30:57 PDT
Yeah I wanted to ask you why we don't check the return value there.
I will come up with another patch.
Comment 4 Gregor Wagner [:gwagner] 2011-04-28 08:56:34 PDT
Created attachment 528863 [details] [diff] [review]
patch
Comment 5 Bill McCloskey (:billm) 2011-05-03 16:15:11 PDT
Comment on attachment 528863 [details] [diff] [review]
patch

Thanks.

Another way to do it is like this:
  if (...is large dense array... && gcmarker->largeStack.push(LargeMarkItem(obj)))
    /* do thing, push succeeded */;
  else
    clasp->trace(gcmarker, obj);

I'm not sure if this is better or worse, but it avoids code duplication.
Comment 6 Gregor Wagner [:gwagner] 2011-05-03 16:34:56 PDT
Yeah I was thinking of the same code but I have never seen it in SM so I didn't want to start with it :)
Comment 7 Gregor Wagner [:gwagner] 2011-05-04 12:56:12 PDT
http://hg.mozilla.org/tracemonkey/rev/ebc5f8a4b233
Comment 8 Chris Leary [:cdleary] (not checking bugmail) 2011-05-10 15:11:17 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/ebc5f8a4b233
Comment 9 Robert Sayre 2011-05-11 14:24:58 PDT
Do we need this fix on Aurora or Beta?
Comment 10 Gregor Wagner [:gwagner] 2011-05-11 14:30:46 PDT
(In reply to comment #9)
> Do we need this fix on Aurora or Beta?

Depends if bug 616666 is in there.
Comment 11 Bill McCloskey (:billm) 2011-05-12 17:48:30 PDT
*** Bug 656761 has been marked as a duplicate of this bug. ***
Comment 12 Gregor Wagner [:gwagner] 2011-05-15 16:49:02 PDT
Bill is the explicit mark stack on Aurora?
Comment 13 Bill McCloskey (:billm) 2011-05-15 17:51:56 PDT
(In reply to comment #12)
> Bill is the explicit mark stack on Aurora?

It seems like Aurora branched on 4/12 and bug 616666 went in on 4/26. So we're good.

It would be nice if there were a web page that listed the time and changeset id of each branch. Maybe there's an easy way to figure this out, but I ended up just searching through hg.mozilla.org, which is pretty annoying.
Comment 14 Dão Gottwald [:dao] 2011-07-09 07:39:10 PDT
> It would be nice if there were a web page that listed the time and changeset
> id of each branch. Maybe there's an easy way to figure this out, but I ended
> up just searching through hg.mozilla.org, which is pretty annoying.

Just let cdleary-bot set the target milestone in bugzilla?

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