Closed Bug 560042 Opened 10 years ago Closed 9 years ago

[OS/2] Build break in js/src/jsgcchunk.cpp

Categories

(Core :: JavaScript Engine, defect)

x86
OS/2
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dave.r.yeo, Assigned: dragtext)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

The build is now breaking here,

I:/comm-central/mozilla/js/src/jsgcchunk.cpp: In function 'void* js::AllocGCChunk()':
I:/comm-central/mozilla/js/src/jsgcchunk.cpp:270: error: 'MapPages' was not decl
ared in this scope
I:/comm-central/mozilla/js/src/jsgcchunk.cpp:275: error: 'UnmapPages' was not declared in this scope
I:/comm-central/mozilla/js/src/jsgcchunk.cpp: In function 'void js::FreeGCChunk(void*)':
I:/comm-central/mozilla/js/src/jsgcchunk.cpp:306: error: 'UnmapPages' was not declared in this scope
The jsgcchunk code assumes that the platform either has a page allocator that can either allocate at the requested alignment or, if that is not available, some API to allocate pages at the given address.
Attached patch MapAlignedPages for OS/2 - v0 (obsolete) — Splinter Review
By design, OS/2 offers no control over an allocation's address or alignment.  I came up with two ways to work around this, and my patch employs both of them.

The first strategy is to allocate a block, and if it isn't aligned properly, allocate another block to fill memory up to the next aligned address.  The function then calls itself and repeats the process recursively until it hits upon an aligned block.  As the recursion unwinds, it frees all of the allocations it didn't use.  If memory is heavily fragmented, this process could go on for too long, so I limit this to 16 attempts.

The fallback strategy is simpler but more expensive:  allocate twice as much space as needed and return an aligned address within this block.  When called upon to unmap a block, the code passes the given address to the appropriate API call.  If that fails, it indicates that the given address was the result of an "expensive" allocation, so it calculates the base address and tries again.

In my limited testing, I haven't seen it require more than 6 recursions to get an aligned block.  However, I tend to spend more time programming this stuff than using it, so I'm not the one to stress test it.  This initial version of the patch contains some printf()'s to identify which strategy was used & how many recursions were needed.  Dave & Walter, could you keep an eye on this?

BTW... none of this is "expensive" in terms of real memory.  Since OS/2 doesn't commit backing storage until a page is touched, we're only talking about filling (or potentially wasting) linear address space here.
Comment on attachment 439819 [details] [diff] [review]
MapAlignedPages for OS/2 - v0

>diff --git a/js/src/jsgcchunk.cpp b/js/src/jsgcchunk.cpp
>--- a/js/src/jsgcchunk.cpp
>+++ b/js/src/jsgcchunk.cpp
>@@ -42,6 +42,11 @@
> # ifdef _MSC_VER
> #  pragma warning( disable: 4267 4996 4146 )
> # endif
>+
>+#elif defined(XP_OS2)
>+
>+# define INCL_DOSMEMMGR
>+# include <os2.h>
> 
> #elif defined(XP_MACOSX) || defined(DARWIN)
> 
>@@ -155,6 +160,107 @@ UnmapPages(void *addr, size_t size)
> }
> 
> # endif /* !WINCE */
>+
>+#elif defined(XP_OS2)
>+
>+#include <stdio.h>
>+#define JS_GC_HAS_MAP_ALIGN 1
>+
>+static void
>+UnmapPages(void *addr, size_t size)
>+{
>+    unsigned long rc = DosFreeMem(addr);
>+    if (!rc) {
>+        if (!(reinterpret_cast<jsuword>(addr) & (size - 1)))
>+            fprintf(stderr, "UnmapPages:  freed addr= %p\n", addr);

Make this DEBUG only if you really want the printf.

>+        return;
>+    }
>+
>+    /* if DosFreeMem() failed, 'addr' is probably part of an "expensive"
>+     * allocation, so calculate the base address and try again */

/*
 *
 */

>+    unsigned long cb = 0x1000;
>+    unsigned long flags;
>+    rc = DosQueryMem(addr, &cb, &flags);
>+    if (rc || cb < size) {
>+        fprintf(stderr, "UnmapPages:  DosQueryMem - rc= %ld  addr= %p  cb= %ld\n",
>+                rc, addr, cb);

Dito.

>+        return;
>+    }
>+
>+    jsuword base = reinterpret_cast<jsuword>(addr) - ((2 * size) - cb);
>+    rc = DosFreeMem(reinterpret_cast<void*>(base));
>+    if (rc)
>+        fprintf(stderr, "UnmapPages:  DosFreeMem - rc= %ld  addr= %p  base= %x\n",
>+                rc, addr, base);
>+    else
>+        fprintf(stderr, "UnmapPages:  freed addr= %p  base= %x\n",
>+                addr, base);
>+

Dito.

>+    return;
>+}
>+
>+static void *
>+MapAlignedPagesRecursively(size_t size, size_t alignment, int& recursions)
>+{
>+    if (++recursions >= 16)
>+        return NULL;

Maybe a define? Nobody likes magic numeric constants.

>+
>+    void *tmp;
>+    if (DosAllocMem(&tmp, size,
>+                    OBJ_ANY | PAG_COMMIT | PAG_READ | PAG_WRITE)) {
>+        JS_ALWAYS_TRUE(DosAllocMem(&tmp, size,
>+                                   PAG_COMMIT | PAG_READ | PAG_WRITE) == 0);
>+    }
>+
>+    int offset = reinterpret_cast<jsuword>(tmp) & (alignment - 1);
>+    if (!offset)
>+        return (tmp);

No extra ()/

>+
>+    void *filler;
>+    if (DosAllocMem(&filler, alignment - offset,
>+                    OBJ_ANY | PAG_COMMIT | PAG_READ | PAG_WRITE)) {
>+        JS_ALWAYS_TRUE(DosAllocMem(&filler, alignment - offset,
>+                                   PAG_COMMIT | PAG_READ | PAG_WRITE) == 0);
>+    }
>+
>+    void * p = MapAlignedPagesRecursively(size, alignment, recursions);

void *p

>+    UnmapPages(tmp, 0);
>+    UnmapPages(filler, 0);
>+
>+    return (p);
>+}
>+
>+static void *
>+MapAlignedPages(size_t size, size_t alignment)
>+{
>+    int recursions = -1;
>+
>+    /* make up to 16 attempts to get an aligned block of the
>+     * right size by recursively allocating blocks of unaligned
>+     * free memory until only an aligned allocation is possible */

/*
 *
 */

>+    void *p = MapAlignedPagesRecursively(size, alignment, recursions);
>+    if (p) {
>+        fprintf(stderr, "MapAlignedPages:  recursions= %d  addr= %p\n",
>+                recursions, p);
>+        return (p);

Extra paren.

>+    }
>+
>+    /* if memory is heavily fragmented, the recursive strategy may fail;
>+     * instead, use the "expensive" strategy:  allocate twice as much
>+     * as requested and return an aligned address within this block */

Comment style.

>+    if (DosAllocMem(&p, 2 * size,
>+                    OBJ_ANY | PAG_COMMIT | PAG_READ | PAG_WRITE)) {
>+        JS_ALWAYS_TRUE(DosAllocMem(&p, 2 * size,
>+                                   PAG_COMMIT | PAG_READ | PAG_WRITE) == 0);
>+    }
>+
>+    jsuword addr = reinterpret_cast<jsuword>(p);
>+    addr = (addr + (alignment - 1)) & ~(alignment - 1);
>+    fprintf(stderr, "MapAlignedPages:  base addr= %p  rtn addr= %x\n",
>+            p, addr);

printf...

>+
>+    return (reinterpret_cast<void *>(addr));
>+}
> 
> #elif defined(XP_MACOSX) || defined(DARWIN)
> 

None of this code is part of our standard builds, and it looks pretty reasonable. You might want to get review from someone else in addition who knows the OS2 memory subsystem.
Attachment #439819 - Flags: review+
Andreas, thanks for the remarkably speedy review but it was premature.  I wouldn't consider submitting a patch for review with raw printf()s and the like (not to mention virtually no testing).  When I'm more confident it works as expected, I'll submit a cleaned-up version for your review.
Great. I just wanted to give you a quick feedback as I was reading the patch. Let me know when you have the final version.
(In reply to comment #2)
> Created an attachment (id=439819) [details]
> MapAlignedPages for OS/2 - v0
> 
[...]
> In my limited testing, I haven't seen it require more than 6 recursions to get
> an aligned block.  However, I tend to spend more time programming this stuff
> than using it, so I'm not the one to stress test it.  This initial version of
> the patch contains some printf()'s to identify which strategy was used & how
> many recursions were needed.  Dave & Walter, could you keep an eye on this?
[...]

In my first test I saw 1/2 a dozen of 1 or 2 recursion then it jumped to 12 to 14 recursions. The second test also four 1 or 2 recursions then
MapAlignedPages:  base addr= 0x25ee6000  rtn addr= 25f00000
and back to 2 recursions.
Attached patch MapAignedPages for OS/2 - v1 (obsolete) — Splinter Review
This fixes a bug that caused UnmapPages() to fail for "expensive" allocations.  It also addresses Andreas's comments regarding style but retains the fprintf's because it still needs more testing.

BTW... I'm going to try some variations on the current allocation scheme to see if I can reduce the number of recursions required and make "expensive" allocations a real rarity.
Attachment #439819 - Attachment is obsolete: true
Attached patch MapAlignedPages for OS/2 - v2 (obsolete) — Splinter Review
This version is a lot smarter than the last one.  When it gets an unaligned block, it confirms there's enough free memory directly above it to allocate both an aligned block and a filler block to use up the space in between.  If not, it just recurses and doesn't create unnecessary filler blocks.

If testing goes well, we'll probably use this version (after removal of the fprintf's).
Attachment #441017 - Attachment is obsolete: true
This version has been working well. SeaMonkey has been very stable and the most recursions I've seen is 9.
Typical output.
MapAlignedPages:  recursions= 1  rtn addr= 0x22200000
MapAlignedPages:  recursions= 1  rtn addr= 0x23500000
MapAlignedPages:  recursions= 3  rtn addr= 0x25600000
MapAlignedPages:  recursions= 3  rtn addr= 0x25700000
MapAlignedPages:  recursions= 1  rtn addr= 0x29100000
MapAlignedPages:  recursions= 1  rtn addr= 0x2cd00000
MapAlignedPages:  recursions= 2  rtn addr= 0x32000000
MapAlignedPages:  recursions= 5  rtn addr= 0x38900000
MapAlignedPages:  recursions= 9  rtn addr= 0x3a100000
UnmapPages:  freed rtn addr= 0x3a100000
To give also some feedback
This is from a recent Minefield build
MapAlignedPages:  recursions= 1  rtn addr= 0x21200000
UnmapPages:  freed rtn addr= 0x21200000
MapAlignedPages:  recursions= 1  rtn addr= 0x21200000
MapAlignedPages:  recursions= 1  rtn addr= 0x21700000
MapAlignedPages:  recursions= 1  rtn addr= 0x21800000
MapAlignedPages:  recursions= 1  rtn addr= 0x21900000
MapAlignedPages:  recursions= 2  rtn addr= 0x2d300000
MapAlignedPages:  recursions= 4  rtn addr= 0x2dd00000
UnmapPages:  freed rtn addr= 0x21200000
UnmapPages:  freed rtn addr= 0x21700000
UnmapPages:  freed rtn addr= 0x21800000
UnmapPages:  freed rtn addr= 0x21900000
UnmapPages:  freed rtn addr= 0x2d300000
UnmapPages:  freed rtn addr= 0x2dd00000
In parallel I was building another mozapp I see rarely more than 2 recursions like here. (2GB memory installed)
(In reply to comment #8)
> Created an attachment (id=441288) [details]
> MapAlignedPages for OS/2 - v2
> 
> If testing goes well, we'll probably use this version (after removal of the
> fprintf's).
Rich, in face of comment #9 and comment #10 you think that your patch can go for check-in?
Attachment #441288 - Attachment is obsolete: true
Attachment #448733 - Flags: review?(gal)
Attachment #448733 - Flags: review?(gal) → review+
Comment on attachment 448733 [details] [diff] [review]
MapAlignedPages for OS/2 - v3 (final)

A couple minor style nits.

>+    /* if DosFreeMem() failed, 'addr' is probably part of an "expensive"
>+     * allocation, so calculate the base address and try again
>+     */

/*
 * If ...
 */
>+    if (DosAllocMem(&tmp, size,
>+                    OBJ_ANY | PAG_COMMIT | PAG_READ | PAG_WRITE)) {

Fits onto one line.

>+        JS_ALWAYS_TRUE(DosAllocMem(&tmp, size,
>+                                   PAG_COMMIT | PAG_READ | PAG_WRITE) == 0);

This too.
(In reply to comment #13)
> (From update of attachment 448733 [details] [diff] [review])
> 
> Fits onto one line.

I was trying to comply with the implied 80 characters/line limit.  The first example would extend to column 86, the second to column 95.  I can change these if you wish but then they'll stick out in more ways than one :-)
The limit is 100 lines now. Its just nits anyway :)
Whiteboard: checkin to TraceMonkey repo
Keywords: checkin-needed
Assignee: general → dragtext
Status: NEW → ASSIGNED
http://hg.mozilla.org/tracemonkey/rev/3a14243ea727
Keywords: checkin-needed
Whiteboard: checkin to TraceMonkey repo → fixed-in-tracemonkey
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.