Need runtime version of MMGC_USE_VIRTUAL_MEMORY and for the false case to be tested

VERIFIED FIXED

Status

Tamarin
Garbage Collection (mmGC)
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Tommy Reilly, Assigned: Tommy Reilly)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

30.10 KB, patch
Lars T Hansen
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Created attachment 369431 [details] [diff] [review]
fixes bug

Symbian folks are dying for this.   runtime flag so its easily testable, always compiling and so mac 10.4 and lower can turn it off.
Attachment #369431 - Flags: review?(lhansen)

Updated

9 years ago
Blocks: 478870

Comment 1

9 years ago
Comment on attachment 369431 [details] [diff] [review]
fixes bug

Reason for rejection: Patch does not apply cleanly to current redux, there's a big block that doesn't fit in GCHeap::Decommit.

Another comment: Are you sure it's a good idea to burden every platform under the sun with having to implement (even stubs for) VM support when there's no gain?  I know MacOS 10.4 is wonky but I don't know why everyone else should suffer because of that.
Attachment #369431 - Flags: review?(lhansen) → review-
(Assignee)

Comment 2

9 years ago
The patch is based on having https://bugzilla.mozilla.org/attachment.cgi?id=369421&action=edit already applied.  

Right now there's only one platform that doesn't use VM support (Symbian) at all so I didn't think it was a big deal.   Also for other platforms there's theories floating around that our VM fanciness doesn't buy us that much, this will enable us to test those theories.

Comment 3

9 years ago
Still rejected, GCHeapGeneric.cpp is missing a definition osSupportsVirtualMemory, suggesting it's not been tested at all.

Other than that the code looks all right.
(Assignee)

Comment 4

9 years ago
Hmm, nice catch.  We don't have any builds that use GCHeapGeneric, I'll think of something...
(Assignee)

Comment 5

9 years ago
Created attachment 369504 [details] [diff] [review]
new patch


Swapped in GCHeapGeneric.cpp on mac and tested it.  Lots of problems with it.  Need to figure out how to build/test it regularly (configure switch?).
Attachment #369431 - Attachment is obsolete: true
Attachment #369504 - Flags: review?(lhansen)

Comment 6

9 years ago
Comment on attachment 369504 [details] [diff] [review]
new patch

No obvious bugs.

For shell builds we could at least compile the generic heap as part of every build on one or all desktop platforms and have a switch to link against it or not; this would require the platform interface to be abstracted out, which is desirable anyway IMO.
Attachment #369504 - Flags: review?(lhansen) → review+
(Assignee)

Comment 7

9 years ago
I'm landing Rishit's patch next and a lot of these static GCHeap methods will become VMPI functions, should be a little cleaner.
(Assignee)

Comment 8

9 years ago
pushed as 1632
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 9

9 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED

Comment 10

9 years ago
Is there a build configuration that QE should work into the buildbot system? (re: comment #6).

Comment 11

9 years ago
(In reply to comment #10)
> Is there a build configuration that QE should work into the buildbot system?
> (re: comment #6).

GCHeapGeneric is gone.  As for the run-time switch it is tested when we're testing OSX 10.4 - it has some quirks and we don't use the VM functions but fall back on malloc/free instead.
You need to log in before you can comment on or make changes to this bug.