NJ merge: get rid of NJ_LOG2_PAGE_SIZE et al

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 402741 [details] [diff] [review]
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).
Attachment #402741 - Flags: review?(graydon)
(Assignee)

Comment 1

9 years ago
Created attachment 402744 [details] [diff] [review]
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.)
Attachment #402741 - Attachment is obsolete: true
Attachment #402744 - Flags: review?(graydon)
Attachment #402741 - Flags: review?(graydon)
(Assignee)

Comment 2

9 years ago
Created attachment 402745 [details] [diff] [review]
patch v3

Attached the wrong patch.  Previous comment applies to this patch (v3).
Attachment #402744 - Attachment is obsolete: true
Attachment #402745 - Flags: review?(graydon)
Attachment #402744 - Flags: review?(graydon)

Updated

9 years ago
Attachment #402745 - Flags: review?(graydon) → review+
(Assignee)

Comment 3

9 years ago
http://hg.mozilla.org/tracemonkey/rev/18edb6af7701
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 4

9 years ago
(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

9 years ago
http://hg.mozilla.org/mozilla-central/rev/18edb6af7701
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.