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

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Tommy Reilly]

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

Comment 1

[Lars T Hansen]

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.

Comment 2

[Tommy Reilly]

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

[Lars T Hansen]

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.

Comment 4

[Tommy Reilly]

Hmm, nice catch.  We don't have any builds that use GCHeapGeneric, I'll think of something...

Comment 5

[Tommy Reilly]

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

Comment 6

[Lars T Hansen]

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.

Comment 7

[Tommy Reilly]

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

Comment 8

[Tommy Reilly]

pushed as 1632

Comment 9

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.

Comment 10

[Brent Baker]

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

Comment 11

[Lars T Hansen]

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