The default bug view has changed. See this FAQ.

JS engine completely broken on ia64

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: glandium, Assigned: Jan Horak)

Tracking

Trunk
mozilla9
Other
Linux
Points:
---

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: No rule to make target `header.py')

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
Everything crashes in the js engine, including jsapi-tests, on ia64. I suspect the new JSVal to be responsible.
(Reporter)

Comment 1

7 years ago
With a debug build, the origin of the problem gets more obvious:
Assertion failure: (objBits >> JSVAL_TAG_SHIFT) == 0, at jsval.h:645

Comment 2

7 years ago
(In reply to comment #1)
> With a debug build, the origin of the problem gets more obvious:
> Assertion failure: (objBits >> JSVAL_TAG_SHIFT) == 0, at jsval.h:645

What is the stack trace?
(Reporter)

Comment 3

7 years ago
(gdb) bt full
#0  0xa000000000010721 in __kernel_syscall_via_break ()
No symbol table info available.
#1  0x20000000000951f0 in raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:46
        _b7 = 0xa000000000010a00
        _out2 = 6
        _arg3 = 6
        _r15 = 1235
        _retval = 0
        _out0 = 7487
        _r8 = <value optimized out>
        _r10 = <value optimized out>
        _out1 = 7487
        pid = <value optimized out>
        res = <value optimized out>
#2  0x4000000000302830 in JS_Assert (s=0x40000000003a02f8 "(objBits >> JSVAL_TAG_SHIFT) == 0", file=0x40000000003a2270 "jsval.h", ln=645) at jsutil.cpp:84
No locals.
#3  0x40000000001a0b60 in OBJECT_TO_JSVAL_IMPL (obj=...) at jsval.h:645
        objBits = 2305843009225244792
#4  js::Value::setObject (obj=...) at jsvalue.h:357
No locals.
#5  ObjectValue (obj=...) at jsvalue.h:692
No locals.
#6  0x40000000001af480 in js_InitClass (cx=0x600000000006fde0, obj=0x2000000000b03000, parent_proto=0x6000000000074430, clasp=0x600000000000ea78, constructor=<value optimized out>, 
    nargs=<value optimized out>, ps=0x0, fs=0x600000000000eba0, static_ps=0x0, static_fs=0x0) at jsobj.cpp:3409
        tvr2 = {<js::AutoGCRooter> = {down = 0x0, tag = 0, context = 0x0}, val = {data = {asBits = 0, debugView = {payload47 = 0, tag = 0}, s = {payload = {i32 = 0, u32 = 0, why = JS_ARRAY_HOLE}}, 
              asDouble = 0}}, _mCheckNotUsedAsTemporary = {mStatementDone = false}}
        key = JSProto_Function
        fun = 0x0
        named = 9
        ctor = <value optimized out>
        atom = 0x2000000000b00340
        proto = 0x2000000000b04000
        tvr = {<js::AutoGCRooter> = {down = 0x0, tag = -11, context = 0x600000000006fde0}, obj = 0x2000000000b04000, _mCheckNotUsedAsTemporary = {mStatementDone = true}}
        scope = <value optimized out>
#7  0x4000000000128d30 in js_InitFunctionClass (cx=0x600000000006fde0, obj=0x2000000000b03000) at jsfun.cpp:2427
        proto = 0x2
        fun = <value optimized out>
#8  0x4000000000052530 in js_InitFunctionAndObjectClasses (cx=0x600000000006fde0, obj=0x2000000000b03000) at jsapi.cpp:1257
        resolving = 0
        rt = 0x20000000006cc010
        key = {obj = 0x2000000000b03000, id = {asBits = 2305843009225229120}}
        table = 0x60000000000741e0
        entry = Cannot access memory at address 0x4d3

Comment 4

7 years ago
(In reply to comment #3)
> #6  0x40000000001af480 in js_InitClass (cx=0x600000000006fde0,
> obj=0x2000000000b03000, parent_proto=0x6000000000074430,
> clasp=0x600000000000ea78, constructor=<value optimized out>, 

Does it mean that IA-64 allows and uses the full range of virtual 64 bit addresses?
(Reporter)

Comment 5

7 years ago
(In reply to comment #4)
> Does it mean that IA-64 allows and uses the full range of virtual 64 bit
> addresses?

Apparently, yes and no.
http://www.informit.com/articles/article.aspx?p=29961&seqNum=2
4.2.4
See virtual address format.

The value for IMPL_VA_MSB is apparently CPU-defined.

Comment 6

7 years ago
(In reply to comment #5)
> http://www.informit.com/articles/article.aspx?p=29961&seqNum=2
> 4.2.4
> See virtual address format.
> 
> The value for IMPL_VA_MSB is apparently CPU-defined.

That value is at least 50 bits per each of 8 regions with text segment and heap pointing to different segments. So on ia64 we either need even fatter values or force the GC thing to come only from 47-bit address range.

Updated

7 years ago
blocking2.0: --- → -

Comment 7

7 years ago
Modifying GC things is much easier than fattening values.  See also the bug for 64-bit Solaris: bug 577056.

Comment 8

7 years ago
(In reply to comment #7)
Err, I meant to say "Modifying the allocation of GC things".

Comment 9

7 years ago
(In reply to comment #8)
> (In reply to comment #7)
> Err, I meant to say "Modifying the allocation of GC things".

On ia64 we would still need to support setting the upper segment bits to non-zero value as both text and mmap pointers have non-zero values there. Given that and the bug 577056 comment 6 I am not sure that we can get away from superfat values if we want to support ia64 or Solaris.

Comment 10

7 years ago
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > Err, I meant to say "Modifying the allocation of GC things".
> 
> On ia64 we would still need to support setting the upper segment bits to
> non-zero value as both text and mmap pointers have non-zero values there.

As long as the high bits are *always* set, that is easy enough to deal with.  However, are there no low level tricks (maybe using sbrk?) that reliably get memory with the high 17-bits clear?

> Given that and the bug 577056 comment 6 I am not sure that we can get away
> from superfat values if we want to support ia64 or Solaris.

See bug 577056 comment 7.  No superfat values.

Comment 11

7 years ago
(In reply to comment #10)
> As long as the high bits are *always* set, that is easy enough to deal with. 
> However, are there no low level tricks (maybe using sbrk?) that reliably get
> memory with the high 17-bits clear?

On ia64 under Linux all pointers has high bits set, see section 4.2.4 at http://www.informit.com/articles/article.aspx?p=29961&seqNum=2 .
(Reporter)

Updated

6 years ago
Duplicate of this bug: 673097
(Assignee)

Comment 13

6 years ago
We can change jemalloc to use mmap for allocation with preferred addr parameter like 0x00000050000000000000. I'm in the middle of this. It's working but I still have problems with short JSFlatStrings (max 2 chars length) staying on 0x200xxxxxxxxxxxxxxxx. Any idea?
(Reporter)

Comment 14

6 years ago
(In reply to jhorak from comment #13)
> We can change jemalloc to use mmap for allocation with preferred addr
> parameter like 0x00000050000000000000.

That sounds really ugly, and doesn't actually guarantee anything.
(Assignee)

Comment 15

6 years ago
(In reply to Mike Hommey [:glandium] from comment #14)
> (In reply to jhorak from comment #13)
> > We can change jemalloc to use mmap for allocation with preferred addr
> > parameter like 0x00000050000000000000.
> 
> That sounds really ugly, and doesn't actually guarantee anything.
So the way to fix it is to enlarge jsval for ia64 and adapt all related issues?

Comment 16

6 years ago
(In reply to jhorak from comment #13)
> It's working but I still have problems with short JSFlatStrings (max 2 chars
> length) staying on 0x200xxxxxxxxxxxxxxxx. Any idea?

The problem here is that some short common strings are allocated in static memory (see unitStaticTable in jsstr.cpp).  To fix this, you either need to convince the compiler to change where it puts static variables or we can work on storing these strings in mmapped memory instead.
(Assignee)

Comment 17

6 years ago
Hm, what about disable short strings tables for ia64 completely? I've managed to run thunderbird just fine with removed static tables. Does this hurt performance/memory a lot?

Changing code to mmap unitStaticTable and other static tables (length2StaticTable, hundredStaticTable, intStaticTable are also affected?) isn't matter of changing few lines. Initialization needs to be placed somewhere (maybe in jsstrinline.h to ensure tables will be initialized before they are used in JSAtom::unitStatic) and we should also free allocated memory before exit.
(Reporter)

Comment 18

6 years ago
(In reply to Luke Wagner [:luke] from comment #16)
> The problem here is that some short common strings are allocated in static
> memory (see unitStaticTable in jsstr.cpp).  To fix this, you either need to
> convince the compiler to change where it puts static variables or we can
> work on storing these strings in mmapped memory instead.

Can the js string api adopt foreign char * pointers ? If so, there is a risk of it using pointers to other static memory.
(Assignee)

Comment 19

6 years ago
(In reply to Mike Hommey [:glandium] from comment #18)
> (In reply to Luke Wagner [:luke] from comment #16)
> > The problem here is that some short common strings are allocated in static
> > memory (see unitStaticTable in jsstr.cpp).  To fix this, you either need to
> > convince the compiler to change where it puts static variables or we can
> > work on storing these strings in mmapped memory instead.
> 
> Can the js string api adopt foreign char * pointers ? If so, there is a risk
> of it using pointers to other static memory.

There is:
JSDependentString::init(JSLinearString *base, const jschar *chars, size_t length)
and
JSExternalString::init(const jschar *chars, size_t length, intN type)
if this is what you mean.
Do you mean static strings outside of js library?
(Reporter)

Comment 20

6 years ago
(In reply to jhorak from comment #19)
> Do you mean static strings outside of js library?

Yes. Like libxul.so (or anything else) giving some static strings to the js library and the js library adopting them instead of copying them. I don't know if there's an API to do that, but if there is, then getting rid of the short common strings table is not going to help.

Comment 21

6 years ago
It's fine if the *chars* are in static memory, the important thing is the JSString header itself because it is the address of the JSString that is boxed into jsval.
(Reporter)

Comment 22

6 years ago
(In reply to Luke Wagner [:luke] from comment #21)
> It's fine if the *chars* are in static memory, the important thing is the
> JSString header itself because it is the address of the JSString that is
> boxed into jsval.

Ah ok. Then getting rid of the static tables would work. Considering IA64 doesn't have a JIT, I don't think the slight performance regression this would cause is worth caring about.

However, I do think we should get rid of these static tables and generate them at initialization time instead, which would save a lot of space for the strings, the corresponding containers, and the corresponding relocations. But that's a different matter. That's in my TODO list.

Comment 23

6 years ago
(In reply to Mike Hommey [:glandium] from comment #22)
> However, I do think we should get rid of these static tables and generate
> them at initialization time instead, which would save a lot of space for the
> strings, the corresponding containers, and the corresponding relocations.
> But that's a different matter. That's in my TODO list.

Agreed.  Its on several TODO lists; let's see gets fed up enough to do it first.  Ideally, we'd lay them out in such a way that the GC didn't have to special case "isStaticAtom" everywhere as it does now.  In theory, we could have mostly-zero initialization overhead by just VirtualAlloc/mmaping the memory and then lazily initializing the strings as necessary.  Although there may be few enough that initialization would be negligible.

Comment 24

6 years ago
(In reply to Luke Wagner [:luke] from comment #23)
> Ideally, we'd lay them out in such a way that the GC didn't have to
> special case "isStaticAtom" everywhere as it does now.

It would be indeed nice to layout those strings in read-nly memory as if they were allocated by the GC including the corresponding mark bitmap that is set to all ones. Then the marking phase of the GC would not need to deal with strings specially.
(Assignee)

Comment 25

6 years ago
Created attachment 553467 [details] [diff] [review]
Patch for thunderbird

Attachment was tested with Thunderbird but it shouldn't differ from Firefox patch. Please have a look and let me know about possible problems (with jemalloc for example).
I'm going to test Firefox now.
(Reporter)

Comment 26

6 years ago
Comment on attachment 553467 [details] [diff] [review]
Patch for thunderbird

Review of attachment 553467 [details] [diff] [review]:
-----------------------------------------------------------------

::: comm-miramar/mozilla/memory/jemalloc/jemalloc.c.ia64
@@ +2260,5 @@
>  	void *ret;
> +#if defined(__ia64__)
> +	/* The ia64 mmap returns pointers with high 48-bits non zero which break jsval.
> +           Setting mmap parameter addr to address with first 48-bits set to zero will 
> +           solve this issue. */

I guess you meant 20 bits.
(Assignee)

Comment 27

6 years ago
Um, sorry 48bits is nonsence, I guess 17 bits should be most suitable, according to:
#define JSVAL_TAG_SHIFT 47

Comment 28

6 years ago
Blech.  Oh well, I guess this is all the more motivation for us to stop allocating strings in static memory.

Instead of doing #if defined(__ia64__) and that comment everywhere, could you put a:

/* the comment */
#if !defined(__ia64__)
# define JS_HAS_STATIC_STRINGS
#endif

right above JSAtom in String.h and then use JS_HAS_STATIC_STRINGS everywhere?  That would also help us test it locally.  Also, I'd guard the actual member declarations/definitions of JSAtom::unitStaticTable et al so that the linker catches you if you miss a use.

Lastly, multi-line comment style is:

/*
 * The ia64 mmap returns..
 */
(Assignee)

Comment 29

6 years ago
Created attachment 554103 [details] [diff] [review]
ia64 patch v0.1

Attaching final patch, please have a look.
Assignee: general → jhorak
Attachment #553467 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #554103 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #554103 - Flags: review? → review?(luke)
(Assignee)

Comment 30

6 years ago
Created attachment 554106 [details] [diff] [review]
ia64 patch v0.2

Sorry, another '48-bits' lured in.
Attachment #554103 - Attachment is obsolete: true
Attachment #554106 - Flags: review?(luke)
Attachment #554103 - Flags: review?(luke)

Comment 31

6 years ago
jhorak: will you be landing this yourself or do you need a checkin?
(Assignee)

Comment 32

6 years ago
I can't checkin. So if you be so kind...

Updated

6 years ago
Attachment #554106 - Flags: review?(luke) → review+

Comment 33

6 years ago
Comment on attachment 554106 [details] [diff] [review]
ia64 patch v0.2

Oops, I didn't see the changes to jemalloc; I think pbiggar should review that.
Attachment #554106 - Flags: review+ → review?(pbiggar)

Comment 34

6 years ago
Created attachment 554203 [details] [diff] [review]
static string changes

Another thing I didn't notice: the patch is pretty out of date.  The attached patch takes a slightly different tack to completely #ifdef off static strings.  This caught a few things missing in the first patch.

jhorak: Could you apply this patch to mozilla-central tip and see if it works for you?  This patch does not include the jemalloc changes (which also need to be rebased).  Could you post a separate patch based on mozilla-central tip and give pbiggar the review?
Attachment #554106 - Attachment is obsolete: true
Attachment #554106 - Flags: review?(pbiggar)
(Assignee)

Comment 35

6 years ago
Yup, I've been doing patch for FF/TB 5, which is my fault. Now I've hit this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=638056
I'm able to build with disabled assembler and jit defines in Makefile but I've got following assert now:
WTF::PageAllocation::allocate (size=4096, usage=UnknownUsage, writable=true, executable=false)
    at /root/tip/js/src/yarr/PageAllocation.h:103
ASSERT(isPageAligned(size));
(gdb) p /x size
$2 = 0x1000
(gdb) p /x pageSize()
$4 = 0x4000

gdb also shows following message when trying to call pageSize():
firefox(32663): unaligned access to 0x00000700070c2584, ip=0x0000070001227480
(Reporter)

Comment 36

6 years ago
(In reply to jhorak from comment #35)
> Yup, I've been doing patch for FF/TB 5, which is my fault. Now I've hit this
> issue: https://bugzilla.mozilla.org/show_bug.cgi?id=638056

You're probably hitting bug 670719, actually.
(Assignee)

Comment 37

6 years ago
U(In reply to Mike Hommey [:glandium] from comment #36)
> (In reply to jhorak from comment #35)
> > Yup, I've been doing patch for FF/TB 5, which is my fault. Now I've hit this
> > issue: https://bugzilla.mozilla.org/show_bug.cgi?id=638056
> 
> You're probably hitting bug 670719, actually.
Um, sorry, I meant: 
https://bugzilla.mozilla.org/show_bug.cgi?id=638056#c11
(Assignee)

Comment 38

6 years ago
Filled bug 680917 to track this issue.
(Assignee)

Comment 39

6 years ago
Created attachment 555080 [details] [diff] [review]
jemalloc patch for ia64

Attaching patch of jemalloc to keep higher 17 bits of allocated memory set to zero which is required for js engine. Not nice, but working.
Attachment #555080 - Flags: review?(pbiggar)
(Assignee)

Comment 40

6 years ago
(In reply to Luke Wagner [:luke] from comment #34)
> jhorak: Could you apply this patch to mozilla-central tip and see if it
> works for you?  This patch does not include the jemalloc changes (which also
> need to be rebased).  Could you post a separate patch based on
> mozilla-central tip and give pbiggar the review?

Together with patch from bug 680917 and attached jemalloc patch works just fine.
(Assignee)

Comment 41

6 years ago
Patch in bug 670719 is also related to this issue.

Updated

6 years ago
Attachment #554203 - Flags: review?(igor)

Comment 42

6 years ago
Comment on attachment 555080 [details] [diff] [review]
jemalloc patch for ia64

Review of attachment 555080 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a solid approach, but just a few things to fix first.

The file in the diff is called tip/memory/jemalloc/jemalloc.c.ia64-jemalloc. As I understand it, this is a patch to jemalloc.c, and you're not adding a new file with an ia64 suffix? Just wanted to check.

::: tip/memory/jemalloc/jemalloc.c.ia64-jemalloc
@@ +2203,5 @@
>  pages_map(void *addr, size_t size, int pfd)
>  {
>  	void *ret;
> +#if defined(__ia64__)
> +	/* The ia64 mmap returns pointers with high 48-bits non zero which break jsval.

I don't understand this comment. I though the issue was that the high 17 bits needed to be clear so we could use them. So why is it 48 bits, and why are the "high" 48-bits non zero. (Perhaps an endian issue of which are "high" and which are not.)

Regardless, this needs a much better comment, explaining the issue.

@@ +2208,5 @@
> +           Setting mmap parameter addr to address with first 48-bits set to zero will 
> +           solve this issue. */
> +	bool check_placement = true;
> +        if (addr == NULL) {
> +		addr = (void*)0x0000070000000000;

This looks like 40 clear bits to me.

@@ +2233,5 @@
>  	if (ret == MAP_FAILED)
>  		ret = NULL;
> +#if defined(__ia64__)
> +        /* Check for wrong memory mapping location only when called with non null addr */
> +	else if (check_placement && addr != NULL && ret != addr) {

This is a weird condition. If check_placement, then addr should always be NULL, so why are we checking that? Much more importantly, this says that we want memory in exactly 0x0000070000000000. That will only succeed once.

If that's the intention (allocate some huge slab o memory to always be reused), then it needs a big comment, and it would also be better to split this functionality out, because that's not what this function is otherwise for. But I dont think this is the intention.

I think you should be checking that the top bits are clear.
Attachment #555080 - Flags: review?(pbiggar) → review-
(Reporter)

Comment 43

6 years ago
(In reply to Paul Biggar from comment #42)
> This is a weird condition. If check_placement, then addr should always be
> NULL, so why are we checking that? Much more importantly, this says that we
> want memory in exactly 0x0000070000000000. That will only succeed once.

I think the idea is that it will work once and return an address below 0x0000070000000000 on subsequent calls, which seems quite optimistic.
(Assignee)

Comment 44

6 years ago
(In reply to Paul Biggar from comment #42)
> Comment on attachment 555080 [details] [diff] [review]
> jemalloc patch for ia64
> 
> Review of attachment 555080 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like a solid approach, but just a few things to fix first.
> 
> The file in the diff is called tip/memory/jemalloc/jemalloc.c.ia64-jemalloc.
> As I understand it, this is a patch to jemalloc.c, and you're not adding a
> new file with an ia64 suffix? Just wanted to check.

Yes, I'm not adding any new files, I use gendiff to generate patch because there is no mercurial package for RHEL5.

> ::: tip/memory/jemalloc/jemalloc.c.ia64-jemalloc
> @@ +2203,5 @@
> >  pages_map(void *addr, size_t size, int pfd)
> >  {
> >  	void *ret;
> > +#if defined(__ia64__)
> > +	/* The ia64 mmap returns pointers with high 48-bits non zero which break jsval.
> 
> I don't understand this comment. I though the issue was that the high 17
> bits needed to be clear so we could use them. So why is it 48 bits, and why
> are the "high" 48-bits non zero. (Perhaps an endian issue of which are
> "high" and which are not.)

You're right, comment is wrong, high 17 bits has to be clear. I'm going to fix it.

> 
> Regardless, this needs a much better comment, explaining the issue.
> 
> @@ +2208,5 @@
> > +           Setting mmap parameter addr to address with first 48-bits set to zero will 
> > +           solve this issue. */
> > +	bool check_placement = true;
> > +        if (addr == NULL) {
> > +		addr = (void*)0x0000070000000000;
> 
> This looks like 40 clear bits to me.

According to mmap docs when addr is specified it's taken as a hint to kernel where to allocate memory. If memory is already taken, value is *increased* until free memory block is found. I don't want to set addr value close to high 17 bits which needs to stay clear as long as addr could be raised and change high 17 bits. I know this is not the robust approach, suggestions/ideas are welcomed.

> 
> @@ +2233,5 @@
> >  	if (ret == MAP_FAILED)
> >  		ret = NULL;
> > +#if defined(__ia64__)
> > +        /* Check for wrong memory mapping location only when called with non null addr */
> > +	else if (check_placement && addr != NULL && ret != addr) {
> 
> This is a weird condition. If check_placement, then addr should always be
> NULL, so why are we checking that? Much more importantly, this says that we
> want memory in exactly 0x0000070000000000. That will only succeed once.

The pages_map is sometimes called with non null addr parameter when caller wants to have allocated memory on specified location (usually somehow aligned). In this case, when memory is not allocated at specified location it needs to be unmapped and NULL returned (caller is aware of that, it change/increase addr and calls pages_map again). The check_placement is set to true in this case. On the other hand I have to specify addr to avoid getting allocated memory with upper 17 bits set and I don't need this check

Checking addr != NULL is really not needed here.

> I think you should be checking that the top bits are clear.
Hm, that's right, assert is not enough I presume...
(Assignee)

Comment 45

6 years ago
(In reply to Mike Hommey [:glandium] from comment #43)
> (In reply to Paul Biggar from comment #42)
> > This is a weird condition. If check_placement, then addr should always be
> > NULL, so why are we checking that? Much more importantly, this says that we
> > want memory in exactly 0x0000070000000000. That will only succeed once.
> 
> I think the idea is that it will work once and return an address below
> 0x0000070000000000 on subsequent calls, which seems quite optimistic.
Unfortunately it does not return address below 0x0000070000000000, but above. The closest free though.
(Assignee)

Comment 46

6 years ago
Created attachment 555361 [details] [diff] [review]
jemalloc patch for ia64 v2

Changes:
- fixed comments
- added check for clear upper 17-bits
Please have a look.
Attachment #555080 - Attachment is obsolete: true
Attachment #555361 - Flags: review?(pbiggar)

Updated

6 years ago
Attachment #554203 - Flags: review?(igor) → review+

Comment 47

6 years ago
Comment on attachment 555361 [details] [diff] [review]
jemalloc patch for ia64 v2

Review of attachment 555361 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks. I have a few suggestions for better comments, which I think you should incorporate.

::: tip/memory/jemalloc/jemalloc.c.ia64-jemalloc
@@ +2212,5 @@
> +         * mmap returns nearest available memory above specified addr. To have enough 
> +         * memory to allocate we set addr to 0x0000070000000000 which shrinks max address 
> +         * space to 0x78ffffffffff. This is enough to cover approx 120 TB of memory.
> +         * See bug #589735 for further info.
> +         */

This comment remains a little unclear, especially as to why those set bits break jsval. Rather than have you guess what I wanted, I rewrote it a little bit. Hope this is useful.

"The JS engine assumes that all allocated pointers have their high 17 bits clear, which ia64's mmap doesn't support directly. However, we can emulate it by passing mmap an "addr" parameter with those bits clear. The mmap will return that address, or the nearest available memory above that address, providing a near-guarantee that those bits are clear. If they are not, we return NULL below to indicate out-of-memory.

addr is chosen as 0x0000070000000000, which still allows about 120TB of virtual address space.

See Bug 589735 for more information."

@@ +2239,5 @@
>  
>  	if (ret == MAP_FAILED)
>  		ret = NULL;
> +#if defined(__ia64__)
> +        /* If allocated memory doesn't have upper 17 bits clear consider it as out of memory */

"If the allocated memory doesn't have its upper 17 bits clear, consider it as out of memory."

@@ +2244,5 @@
> +        else if ((long long)ret & 0xffff800000000000) {
> +		munmap(ret, size);
> +                ret = NULL;
> +        }
> +        /* Check for wrong memory mapping location only when called with non null addr */

"If the caller requested a specific memory location, verify that's what mmap returned."
Attachment #555361 - Flags: review?(pbiggar) → review+
(Assignee)

Comment 48

6 years ago
Feel free to modify my comments. I think yours express much better what's is this patch all about.
(Assignee)

Comment 49

6 years ago
Created attachment 555720 [details] [diff] [review]
jemalloc patch for ia64 v3

Comments changed according to comment #47. Thanks a lot for cooperation. Setting review+ as long as only comments changed.
Attachment #555361 - Attachment is obsolete: true
Attachment #555720 - Flags: review+

Comment 50

6 years ago
Landed the static strings patch:
http://hg.mozilla.org/integration/mozilla-inbound/rev/00be7279f6ad
http://hg.mozilla.org/mozilla-central/rev/00be7279f6ad

Leaving open for jemalloc patch.
Keywords: checkin-needed
Whiteboard: [checkin-needed for jemalloc patch]
Target Milestone: --- → mozilla9
I'd really appreciate a user string and checkin comment to use for pushing the jemalloc patch...
The jemalloc patch is also malformed (or at least will be a pain to apply), eg source and target filenames are different, isn't in git format, path has tip/ prepended etc.

jhorak, please can you set up your hgrc as here / add author commit message (including the correct r=foo):
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Thanks :-)
Keywords: checkin-needed
Whiteboard: [checkin-needed for jemalloc patch] → [left open for jemalloc patch]
(Reporter)

Comment 54

6 years ago
FWIW, a 6.0 build with a backport of these patches more or less passes reftests and crashtest, but crash on xpcshell tests and jstestbrowser.
(Assignee)

Comment 55

6 years ago
(In reply to Mike Hommey [:glandium] from comment #54)
> FWIW, a 6.0 build with a backport of these patches more or less passes
> reftests and crashtest, but crash on xpcshell tests and jstestbrowser.
Patch from bug 680917 should fix this issue.
(Reporter)

Comment 56

6 years ago
(In reply to jhorak from comment #55)
> (In reply to Mike Hommey [:glandium] from comment #54)
> > FWIW, a 6.0 build with a backport of these patches more or less passes
> > reftests and crashtest, but crash on xpcshell tests and jstestbrowser.
> Patch from bug 680917 should fix this issue.

6.0 doesn't have the bump pointer allocator.
(Assignee)

Comment 57

6 years ago
Created attachment 556505 [details] [diff] [review]
jemalloc patch for ia64 v4

Patch generated using hg diff.
Attachment #555720 - Attachment is obsolete: true
Attachment #556505 - Flags: review+
Thanks for updating the patch. There still wasn't a commit message and/or author (I had to end up googling "jhorak redhat" to even find your real name, since the field isn't set on bugzilla), but I've just gone and added it now to save another back and forth. 

The jemalloc part is now in my queue with a few other bits that are being sent to try first and then onto inbound.
(Assignee)

Comment 59

6 years ago
(In reply to Ed Morley [:edmorley] from comment #58)
> Thanks for updating the patch. There still wasn't a commit message and/or
> author (I had to end up googling "jhorak redhat" to even find your real
> name, since the field isn't set on bugzilla), but I've just gone and added
> it now to save another back and forth. 

Sorry for complication. I haven't used qnew etc before. It seems to be clear to me now.

> FWIW, a 6.0 build with a backport of these patches more or less passes reftests and crashtest, 
> but crash on xpcshell tests and jstestbrowser.

Does xpcshell and jstestbrowser use jemalloc or system's malloc? Do you have the backported patch somewhere that I can retest with ia64 without need to do backport (not sure how complex it is)? Thanks.
(Reporter)

Comment 60

6 years ago
(In reply to jhorak from comment #59)
> (In reply to Ed Morley [:edmorley] from comment #58)
> > Thanks for updating the patch. There still wasn't a commit message and/or
> > author (I had to end up googling "jhorak redhat" to even find your real
> > name, since the field isn't set on bugzilla), but I've just gone and added
> > it now to save another back and forth. 
> 
> Sorry for complication. I haven't used qnew etc before. It seems to be clear
> to me now.
> 
> > FWIW, a 6.0 build with a backport of these patches more or less passes reftests and crashtest, 
> > but crash on xpcshell tests and jstestbrowser.
> 
> Does xpcshell and jstestbrowser use jemalloc or system's malloc? Do you have
> the backported patch somewhere that I can retest with ia64 without need to
> do backport (not sure how complex it is)? Thanks.

http://anonscm.debian.org/gitweb/?p=pkg-mozilla/iceweasel.git;a=blob;f=debian/patches/porting/Bug-589735-Allow-static-JS-strings-to-be-turned-off-.patch;h=0b6919f698c625a26a960c96ae612abf798ec561;hb=bcc620620266b4a720621446ab7e63fe2260e3b3

jstestbrowser is using reftest, so it must be using the firefox binary, with jemalloc. xpcshell is without, and that would explain that all tests crash. FWIW, bug 680440 is going to change that.
http://hg.mozilla.org/integration/mozilla-inbound/rev/9c15d0fb3e25
Whiteboard: [left open for jemalloc patch]
http://hg.mozilla.org/mozilla-central/rev/9c15d0fb3e25
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Whiteboard: No rule to make target `header.py'
You need to log in before you can comment on or make changes to this bug.