Closed Bug 673197 Opened 13 years ago Closed 13 years ago

Enable jemalloc on VC8/9 express

Categories

(Core :: Memory Allocator, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(3 files)

Currently jemalloc requires VC8/9 pro or VC10. It should be possible to enable new-style jemalloc on VC8/9 express too.
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...)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #547466 - Flags: review?(khuey)
Comment on attachment 547466 [details] [diff] [review]
Proposed patch
[Checked in: See comment 13]

Awesome stuff.
Attachment #547466 - Flags: review?(khuey) → review+
Pushed changeset 83ef35b794ce to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Backed out:

http://hg.mozilla.org/mozilla-central/rev/a627b24e684e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
vc10 jemalloc build has been collapsed after back out.
Attachment #549626 - Flags: review?(Ms2ger)
if use -MANIFEST:NO, vc8/9 build crashes.
vc10 is no probrem without -MANIFEST:NO.
(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 on attachment 549626 [details] [diff] [review]
fix back out misstake
[Checked in: Comment 11]

Oops, thanks for spotting this!
Attachment #549626 - Flags: review?(Ms2ger) → review+
(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.
(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.
(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.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
(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.
Blocks: 681908
Attachment #549626 - Attachment description: fix back out misstake. → fix back out misstake [Checked in: Comment 14]
Attachment #549626 - Attachment description: fix back out misstake [Checked in: Comment 14] → fix back out misstake [Checked in: Comment 11]
Attachment #547466 - Attachment description: Proposed patch → Proposed patch [Backed out: Comment 4]
Attachment #547466 - Attachment is obsolete: true
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
Attachment #547466 - Attachment description: Proposed patch [Backed out: Comment 4] → Proposed patch [Checked in: See comment 13]
Attachment #547466 - Attachment is obsolete: false
You need to log in before you can comment on or make changes to this bug.