589735 - JS engine completely broken on ia64

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Hommey [:glandium]]

Everything crashes in the js engine, including jsapi-tests, on ia64. I suspect the new JSVal to be responsible.

Comment 1

[Mike Hommey [:glandium]]

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

[Igor Bukanov]

(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?

Comment 3

[Mike Hommey [:glandium]]

(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

[Igor Bukanov]

(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?

Comment 5

[Mike Hommey [:glandium]]

(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

[Igor Bukanov]

(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.

Comment 7

[Luke Wagner [:luke]]

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

Comment 8

[Luke Wagner [:luke]]

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

Comment 9

[Igor Bukanov]

(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

[Luke Wagner [:luke]]

(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

[Igor Bukanov]

(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 .

Comment 13

[Jan Horak [:jhorak]]

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?

Comment 14

[Mike Hommey [:glandium]]

(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.

Comment 15

[Jan Horak [:jhorak]]

(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

[Luke Wagner [:luke]]

(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.

Comment 17

[Jan Horak [:jhorak]]

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.

Comment 18

[Mike Hommey [:glandium]]

(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.

Comment 19

[Jan Horak [:jhorak]]

(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?

Comment 20

[Mike Hommey [:glandium]]

(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

[Luke Wagner [:luke]]

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.

Comment 22

[Mike Hommey [:glandium]]

(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

[Luke Wagner [:luke]]

(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

[Igor Bukanov]

(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.

Comment 25

[Jan Horak [:jhorak]]

Attachment: 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.

Comment 26

[Mike Hommey [:glandium]]

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.

Comment 27

[Jan Horak [:jhorak]]

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

Comment 28

[Luke Wagner [:luke]]

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..
 */

Comment 29

[Jan Horak [:jhorak]]

Attachment: ia64 patch v0.1

Attaching final patch, please have a look.

Comment 30

[Jan Horak [:jhorak]]

Attachment: ia64 patch v0.2

Sorry, another '48-bits' lured in.

Comment 31

[Luke Wagner [:luke]]

jhorak: will you be landing this yourself or do you need a checkin?

Comment 32

[Jan Horak [:jhorak]]

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

Comment 33

[Luke Wagner [:luke]]

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.

Comment 34

[Luke Wagner [:luke]]

Attachment: 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?

Comment 35

[Jan Horak [:jhorak]]

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

Comment 36

[Mike Hommey [:glandium]]

(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.

Comment 37

[Jan Horak [:jhorak]]

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

Comment 38

[Jan Horak [:jhorak]]

Filled bug 680917 to track this issue.

Comment 39

[Jan Horak [:jhorak]]

Attachment: 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.

Comment 40

[Jan Horak [:jhorak]]

(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.

Comment 41

[Jan Horak [:jhorak]]

Patch in bug 670719 is also related to this issue.

Comment 42

[Paul Biggar]

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.

Comment 43

[Mike Hommey [:glandium]]

(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.

Comment 44

[Jan Horak [:jhorak]]

(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...

Comment 45

[Jan Horak [:jhorak]]

(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.

Comment 46

[Jan Horak [:jhorak]]

Attachment: jemalloc patch for ia64 v2

Changes:
- fixed comments
- added check for clear upper 17-bits
Please have a look.

Comment 47

[Paul Biggar]

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."

Comment 48

[Jan Horak [:jhorak]]

Feel free to modify my comments. I think yours express much better what's is this patch all about.

Comment 49

[Jan Horak [:jhorak]]

Attachment: jemalloc patch for ia64 v3

Comments changed according to comment #47. Thanks a lot for cooperation. Setting review+ as long as only comments changed.

Comment 50

[Luke Wagner [:luke]]

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

Comment 51

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/00be7279f6ad

Leaving open for jemalloc patch.

Comment 52

[Boris Zbarsky [:bzbarsky]]

I'd really appreciate a user string and checkin comment to use for pushing the jemalloc patch...

Comment 53

[Ed Morley [:emorley]]

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 :-)

Comment 54

[Mike Hommey [:glandium]]

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.

Comment 55

[Jan Horak [:jhorak]]

(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.

Comment 56

[Mike Hommey [:glandium]]

(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.

Comment 57

[Jan Horak [:jhorak]]

Attachment: jemalloc patch for ia64 v4

Patch generated using hg diff.

Comment 58

[Ed Morley [:emorley]]

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.

Comment 59

[Jan Horak [:jhorak]]

(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.

Comment 60

[Mike Hommey [:glandium]]

(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.

Comment 61

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9c15d0fb3e25

Comment 62

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/9c15d0fb3e25