518747 - NJ merge: get rid of NJ_LOG2_PAGE_SIZE et al

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch removes NJ_LOG2_PAGE_SIZE from all the backends, to match
Tamarin.  jstracer was using it via NJ_PAGE_SIZE for the Tracker, so I
introduced a new constant TRACKER_PAGE_SIZE which is now used instead.  Its
size is 4096 which the same as NJ_PAGE_SIZE for all backends except Sparc,
so Tracker lookup performance shouldn't be affected.

I also changed "Page" to "TrackerPage" throughout, because I don't like the
idea of taking a well-defined existing term like "Page" (as in OS pages) and
using it to refer to something similar-but-subtly-different (arbitrarily
sized memory chunks in our own data structure).

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch v2

In the new version I tweaked a comment, and renamed PAGESIZE as TRACKER_PAGE_SIZE.

But now I'm not sure about this comment:

 * The size of the map is allocated as N * sizeof(LIns*), where N is
 * (TRACKER_PAGE_SIZE >> 2).  Since the lower two bits are 0, they are always
 * discounted.  The result is the map can store N pointers.
 *
 * PAGEMASK is the "reverse" expression, with a |- 1| to get a mask which
 * separates an address into the Base and Index bits. It is necessary to do
 * all this work rather than use TRACKER_PAGE_SIZE - 1, because on 64-bit
 * platforms the pointer width is twice as large, and only half as many
 * indexes can fit into TrackerPage::map. So the "Base" grows by one bit, and
 * the "Index" shrinks by one bit.

In particular, the "on 64-bit... only half as many indexes can fit into TrackerPage::map" doesn't seem right, because we allocate twice as much space on 64 bit machines.  As a result, I'm not sure if the last sentence is right, and thus whether Tracker works properly on 64-bit machines.  (Nb: this is all independent of the patch, which hasn't changed the operation at all... any problems were pre-existing.)

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch v3

Attached the wrong patch.  Previous comment applies to this patch (v3).

Comment 3

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/18edb6af7701

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> 
> In particular, the "on 64-bit... only half as many indexes can fit into
> TrackerPage::map" doesn't seem right, because we allocate twice as much space
> on 64 bit machines.  As a result, I'm not sure if the last sentence is right,
> and thus whether Tracker works properly on 64-bit machines.

Bug 519156 is the follow-up for this observation.

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/18edb6af7701

Comment 6

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/57e1fe5bcc56