Last Comment Bug 673197 - Enable jemalloc on VC8/9 express
: Enable jemalloc on VC8/9 express
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Memory Allocator (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: 681908 694371
  Show dependency treegraph
 
Reported: 2011-07-21 12:14 PDT by neil@parkwaycc.co.uk
Modified: 2011-10-21 09:00 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch [Checked in: See comment 13] (4.39 KB, patch)
2011-07-21 12:20 PDT, neil@parkwaycc.co.uk
khuey: review+
Details | Diff | Splinter Review
fix back out misstake [Checked in: Comment 11] (915 bytes, patch)
2011-07-30 23:00 PDT, ABE Hiroki (:hATrayflood)
neil: review+
Details | Diff | Splinter Review
fix vc8/9 crashes. (1.09 KB, patch)
2011-07-30 23:38 PDT, ABE Hiroki (:hATrayflood)
no flags Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2011-07-21 12:14:44 PDT
Currently jemalloc requires VC8/9 pro or VC10. It should be possible to enable new-style jemalloc on VC8/9 express too.
Comment 1 neil@parkwaycc.co.uk 2011-07-21 12:20:12 PDT
Created attachment 547466 [details] [diff] [review]
Proposed patch
[Checked in: See comment 13]

This patch searches for the VC8/9 source. Only if it doesn't find it does it use the new-style jemalloc. This means that it doesn't affect Tinderbox.

Because of the crtdll check, this actually works on debug VC8/9 express builds  and debug and opt Windows SDK 6/7/7.1 x86/x64 builds too. (Well, SDK 6 is currently broken for other reasons...)
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-25 10:43:57 PDT
Comment on attachment 547466 [details] [diff] [review]
Proposed patch
[Checked in: See comment 13]

Awesome stuff.
Comment 3 neil@parkwaycc.co.uk 2011-07-27 13:04:20 PDT
Pushed changeset 83ef35b794ce to mozilla-central.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2011-07-27 14:27:21 PDT
Backed out:

http://hg.mozilla.org/mozilla-central/rev/a627b24e684e
Comment 5 ABE Hiroki (:hATrayflood) 2011-07-30 23:00:13 PDT
Created attachment 549626 [details] [diff] [review]
fix back out misstake
[Checked in: Comment 11]

vc10 jemalloc build has been collapsed after back out.
Comment 6 ABE Hiroki (:hATrayflood) 2011-07-30 23:38:47 PDT
Created attachment 549630 [details] [diff] [review]
fix vc8/9 crashes.

if use -MANIFEST:NO, vc8/9 build crashes.
vc10 is no probrem without -MANIFEST:NO.
Comment 7 neil@parkwaycc.co.uk 2011-07-31 02:32:23 PDT
(In reply to comment #6)
> if use -MANIFEST:NO, vc8/9 build crashes.
> vc10 is no probrem without -MANIFEST:NO.

vc8/9 build shouldn't be using that line.
Comment 8 neil@parkwaycc.co.uk 2011-07-31 02:35:49 PDT
Comment on attachment 549626 [details] [diff] [review]
fix back out misstake
[Checked in: Comment 11]

Oops, thanks for spotting this!
Comment 9 ABE Hiroki (:hATrayflood) 2011-07-31 02:38:25 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > if use -MANIFEST:NO, vc8/9 build crashes.
> > vc10 is no probrem without -MANIFEST:NO.
> 
> vc8/9 build shouldn't be using that line.

sorry, vc8/9 **express** jemalloc build crashes.
Comment 10 ABE Hiroki (:hATrayflood) 2011-07-31 02:47:31 PDT
(In reply to comment #8)
> Comment on attachment 549626 [details] [diff] [review] [diff] [details] [review]
> fix back out misstake.
> 
> Oops, thanks for spotting this!

thanks.
this patch solves Bug 674972.
Comment 11 neil@parkwaycc.co.uk 2011-07-31 02:55:39 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > Oops, thanks for spotting this!
> 
> thanks.
> this patch solves Bug 674972.

Pushed changeset f92e021f1f44 to mozilla-central.

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > if use -MANIFEST:NO, vc8/9 build crashes.
> > > vc10 is no probrem without -MANIFEST:NO.
> > 
> > vc8/9 build shouldn't be using that line.
> 
> sorry, vc8/9 **express** jemalloc build crashes.

Ah, I realise my mistake, the crtdll.obj bug doesn't affect debug builds, but release builds have the problem.

Please can you file a new bug on vc8/9 express release builds? You'll need to ask khuey to review your patch.
Comment 12 ABE Hiroki (:hATrayflood) 2011-07-31 13:50:48 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > Oops, thanks for spotting this!
> > 
> > thanks.
> > this patch solves Bug 674972.
> 
> Pushed changeset f92e021f1f44 to mozilla-central.
> 
thanks.

> (In reply to comment #9)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > if use -MANIFEST:NO, vc8/9 build crashes.
> > > > vc10 is no probrem without -MANIFEST:NO.
> > > 
> > > vc8/9 build shouldn't be using that line.
> > 
> > sorry, vc8/9 **express** jemalloc build crashes.
> 
> Ah, I realise my mistake, the crtdll.obj bug doesn't affect debug builds,
> but release builds have the problem.
> 
> Please can you file a new bug on vc8/9 express release builds? You'll need
> to ask khuey to review your patch.

ok. I filed a new Bug 675519.
Comment 13 Serge Gautherie (:sgautherie) 2011-10-21 08:59:18 PDT
Comment on attachment 547466 [details] [diff] [review]
Proposed patch
[Checked in: See comment 13]

Oh, a modified version of this patch was eventually checked in:
http://hg.mozilla.org/mozilla-central/rev/7bc488fc53a3

Note You need to log in before you can comment on or make changes to this bug.