480141 - Integration from tr-oom branch

Status: VERIFIED FIXED

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

Description

[Tommy Reilly]

Attachment: diff

This is all my saved up work that wasn't ready for the last sprint.  Change comment:

Mergin in tr-oom branch.  Highlights:

Further refinements to OOM support
Profiler improvements
Flair build changes (ie, use _t types everywhere)
pyspy profiler trigger
ifdef removal (WRITER_BARRIERS, MMGC_DRC) and cleanup

Comment 1

[Tommy Reilly]

fixes bugs: 412309, 431700

Comment 2

[Tommy Reilly]

Can I get folks to review this like so:

Erik: MMgc through GCHeap.cpp
Lars: GCHeap.cpp/.h
Steven: GCHeapGeneric.cpp through to nanojit stuff
Edwin: from nanojit/Assembler.cpp to the end 

I realize its big sorry, next time I'll try to break things up.  tamarin-redux was in end of sprint lock down when most of this work was done.

Comment 3

[Tommy Reilly]

My change notes are incomplete, here's a fuller version:

Mergin in tr-oom branch.  Highlights:

Incorporated AIR GCHeap features into ours:
  1) more aggressive Decommit, we decomit to maintain at most %25 (configurable) of the heap free
  2) Decommit done automatically after each sweep
  3) optional region favoring in AllocBlock, ie use oldest region (used only on OS X <= 10.4)
  4) optional trimming of virtual memory (done on all 32 bit OSes at the moment) we remove completely empty regions when our free virtual space is greater than %50 (configurable) of total heap size
  5) working Decommit on Mac, all versions, functionality is limited on OS X <= 10.4
  6) optional allocation spanning of regions (again for OS X <= 10.4), ie memory from OS is never treated as contiguous even if it is
  7) large allocations that result in heap expansion are completely tossed out upon being freed instead of being retained

Further refinements to OOM support
Profiler improvements
Flair build changes (ie, use _t types everywhere)
pyspy profiler trigger
ifdef removal (WRITER_BARRIERS, MMGC_DRC) and cleanup

Comment 4

[Steven Johnson]

Comment on attachment 364115 [details] [diff] [review]
diff


A few nits:

-- #include "../platform/VMPI.h" should not need the relative path, just "VMPI.h" should suffice everywhere (and is preferable)

-- Win32 APIs need to use explicit "A" or "W" suffixes so that they will compile correctly regardless of whether UNICODE is defined or not... e.g. FormatMessage, CreateNamedPipe, etc may need tweaking.

-- what's up with 
	
	#pragma warning(disable:4512) //assignment operator could not be generated

That's a warning I'd want to know more about, it might be significant. (And usually you can work around it by creating the assignment operator yourself.)

-- CodegenLIR.cpp:165: #define FUNCADDR is now the same both ways, no ifdef is needed anymore

-- PoolObject.h:62: we should probably prefer to use "uint8_t" over "byte" for new code

-- in RCPtr, we have repeated use of

	if (t && (uintptr_t)t != 1) ...

but we could do this in a single compare with:

	if (uintptr_t(t) > 1) ...

Comment 5

[Tommy Reilly]

RE:  #pragma warning(disable:4512) //assignment operator could not be generated

This is already done for MSVC builds, I'm just doing it for ARM builds too now.

RE: #include "../platform/VMPI.h" 

+1 I must have not had updated include paths when I first did, will fix

RE: Win32 APIs need to use explicit "A" or "W"

+1 will add the A

RE: CodegenLIR.cpp:165: #define FUNCADDR is now the same both ways, no ifdef is
needed anymore

good catch

RE: -- PoolObject.h:62: we should probably prefer to use "uint8_t" over "byte" for
new code

sure

RE: RCPtr 

+1

Comment 6

[Tommy Reilly]

http://hg.mozilla.org/users/treilly_adobe.com/tr-oom/rev/9ef741d7897c

Comment 7

[Rick Reitmaier]

Not that important but just curious why you removed the mem tag from Fragmento.cpp - MMGC_MEM_TYPE("NanoJitMem");

Comment 8

[Tommy Reilly]

1) MMGC_MEM_TYPE tags work only on new/GC::Alloc, not GCHeap::Alloc 

2) The new profiler uses the function name to tag an alloc and I deemed "nanojit::Fragmento::pagesGrow" as a meaningful enough tag

Comment 9

[Lars T Hansen]

Comment on attachment 364115 [details] [diff] [review]
diff

GCHeap.h:

The unit (bytes, kBytes, etc) for kDefaultReserve needs to be specified.  Encoding the unit in the name is best.

This comment does not parse:
// some OS's are loose with how with virtual memory and we don't have to track
// each region individually

GCHeap.cpp:

Portability no-no: file includes ISO C headers, leave this to VMPI.h.  (This probably means you depend on definitions imported from those headers too, like stdio functions - not good.  Indeed there are calls to fprintf, at least.)

This comment does not parse:
// for investigative purposes, does turning this off may increase our ability un-reserve memory

This comment ends abruptly:
// search from the end of the free list so we decommit big blocks, if
// a free block is bigger than

There are calls to alloca in GCHeap::DumpHeapRep, but that's not portable and the function is not protected by any kind of #ifdef DESKTOP_DEBUGGING (for instance).

r+ but only on the condition that the portability problems are logged as a bug that blocks the next release (https://bugzilla.mozilla.org/show_bug.cgi?id=478870)

Comment 10

[Tommy Reilly]

http://hg.mozilla.org/users/treilly_adobe.com/tr-oom/rev/bf669c734dd0

Comment 11

[Tommy Reilly]

pushed, Edwin was taking to long and his part of the diff was just ifdef line removals.

Comment 12

[Edwin Smith]

(In reply to comment #2)

> I realize its big sorry, next time I'll try to break things up.  tamarin-redux
> was in end of sprint lock down when most of this work was done.

Yup.  lockdown has nothing to do with it... mercurial queues, named branches, or separate repos are all fine ways of keeping things separate while working on them all at once, indefinitely.  (i like named branches best, but whatever works for you...)

VMPI.h - is adding namespace avmplus {} here in this file blessed for portability?  I doubt it.

avmshell.cpp - comment on ~line 80 doesn't make sense here -- maybe it
it should point the reader to MMgc/GlobalNew.h?

heap_stress.as, pyspy - need copyright comments.  

LIR.cpp - commented out code braketed by #ifdef MMGC_MEMORY_INFO should be uncommented and debugged, or deleted

GrowableBuffer.cpp - this patch must be old because that file is gone now

generally: pyspy is neat, but strikes me as another debugging tool that will quickly grow stale because only one person knows how to use it/maintain it.  (debugging tools: good!  up-to-date documented, reliable tools: priceless).

Comment 13

[Tommy Reilly]

Points taken, need to spend some time learning about queues and named branches.

The stuff in the namespace avmplus are the legacy types, they were in that namespace before VMPI existed, I just put them back so the player would compile, the goal is for them to go away.  Ie, re-introducing those types in the global namespace was an un-intentational mistake.

Dan Schaffer is working on acceptance tests for pyspy.  I need to update the MDC docs for it.

I'll post another patch to address the other issues.

Comment 14

[Chris Peyer]

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