Last Comment Bug 675545 - Completely re-do jsarena.{cpp,h}
: Completely re-do jsarena.{cpp,h}
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
: 420476 (view as bug list)
Depends on:
Blocks: 675150
  Show dependency treegraph
 
Reported: 2011-07-31 17:57 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2011-08-30 04:32 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (14.73 KB, patch)
2011-08-01 00:02 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
cdleary: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-31 17:57:37 PDT
jsarena.{cpp,h} are *awful*.  Bad things I've discovered so far:

- Bug 675150.

- In multiple places we compare |nb| (the size of an allocation request) against |a->limit| (a pointer).  WTF?

- There's this optimization where larger-than-arena-size allocations get an extra pointer in their header so that if they are reallocated, we can avoid searching through the list of JSArenas.  But it's hardly ever hit (once in all of SunSpider, a few dozen times in the browser when starting up and loading Gmail).  But it complicates things massively.

Seriously, I just started out wanting to fix bug 675150, but I'm having trouble seeing how to do incremental fixes, the whole thing is so busted.

This code should be about half as long as it currently is.
Comment 1 Luke Wagner [:luke] 2011-07-31 20:08:01 PDT
Please do.  Chris had a patch from a year ago that redid things in a template-y manner.

Of historical interest, jsarena used to support inline function calls, invoke args, and a few other things before these were all redone.  That may explain some of the hackage.
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-31 21:57:08 PDT
(In reply to comment #0)
> 
> - In multiple places we compare |nb| (the size of an allocation request)
> against |a->limit| (a pointer).  WTF?

Oh god, turns out this is necessary to avoid wrap-arounds in some cases.  There's no comment explaining it, of course :(
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-31 21:57:46 PDT
(In reply to comment #1)
> Please do.  Chris had a patch from a year ago that redid things in a
> template-y manner.

Yeah, bug 559408.  It landed and bounced, but got marked RESOLVE FIXED nonetheless.
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-08-01 00:02:40 PDT
Created attachment 549723 [details] [diff] [review]
patch

Changes in this patch:

- Each oversized block had an extra pointer in its header which allowed its
  arena to be found in O(1) time;  this was used if it was realloc'd.  This
  turned out to not help in practice, AFAICT, and was hugely complex, so I
  removed it.

- It restricts the requested alignments to 1, 2, 4 or 8.  This avoids a lot of
  mucking around with post-header alignment code, because the header is
  always 8-aligned.  (Multiple assertions guarantee this.)

- Adds some comments.

Passes jstests and jit-tests;  I'll do a full try server run before any landing.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-08-02 18:21:42 PDT
*** Bug 420476 has been marked as a duplicate of this bug. ***
Comment 6 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-08-28 23:16:07 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f9638b44893b
Comment 7 Ed Morley [:emorley] 2011-08-30 04:32:41 PDT
http://hg.mozilla.org/mozilla-central/rev/f9638b44893b

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