Remove MMGC_HOOKS, MMGC_MEMORY_PROFILER, DEBUGGER, and AVMPLUS_SAMPLER (in MMgc)

ASSIGNED
Assigned to

Status

Tamarin
Garbage Collection (mmGC)
P2
normal
ASSIGNED
7 years ago
6 years ago

People

(Reporter: Lars T Hansen, Assigned: Tony Printezis)

Tracking

(Blocks: 1 bug)

unspecified
Q1 12 - Brannan
Bug Flags:
flashplayer-qrb +

Details

(Whiteboard: PACMAN)

(Reporter)

Description

7 years ago
MMGC_HOOKS, MMGC_MEMORY_PROFILER, and AVMPLUS_SAMPLER can all be supported as run-time selectable options in MMgc with zero overhead in the case where hooks/profiler/sampler are not enabled, and modest cost when they are.  The trick is to push the necessary tests onto slow paths in the allocators and make sure the slow paths are taken when the features are enabled.  In GCAlloc this is straightforward; in FixedAlloc I don't know yet.

Removing them has several benefits:

- makes DEBUGGER runs significantly cheaper (see eg bug 528797)
- helps get rid of the DEBUGGER vs non-DEBUGGER configuration split; we can
  always support a debugger and it's always affordable
- make the code a lot simpler
- open up a path to a better allocator (by making the code simpler)
(Reporter)

Updated

7 years ago
Duplicate of this bug: 640152
(Reporter)

Updated

7 years ago
Duplicate of this bug: 528797
(Reporter)

Comment 3

7 years ago
The patch queue is at 
hg.mozilla.org/users/lhansen_adobe.com/redux-debugger-for-free.
(Reporter)

Updated

7 years ago
Summary: Remove MMGC_HOOKS, MMGC_MEMORY_PROFILER, and AVMPLUS_SAMPLER (in MMgc) → Remove MMGC_HOOKS, MMGC_MEMORY_PROFILER, DEBUGGER, and AVMPLUS_SAMPLER (in MMgc)
(Reporter)

Comment 4

7 years ago
The queue is complete (all the #ifdefs have been eradicated completely from MMgc, code compiles on Mac and Windows, and Tamarin passes local Mac testing) and ready for in-depth testing of affected features and performance.

Updated

7 years ago
Flags: flashplayer-qrb+
(Reporter)

Comment 5

7 years ago
From an AIR perspective this is a PACMAN item because AIR is always built Release-Debugger.
Whiteboard: PACMAN
(Reporter)

Updated

7 years ago
Assignee: lhansen → fklockii
(In reply to Lars T Hansen from comment #4)

I spent some time rebasing the queue to apply atop TR 7184:0d46338d1354

I'm not convinced the queue was properly tested in all contexts.  Here are two problems in particular:

1.) samplerCleanup removes the AVMPLUS_SAMPLER preprocessor symbol, which leaves this code exposed with no #ifdef guard:

            Sampler* sampler = declaringTraits ? declaringTraits->core->get_sampler() : NULL;
            if (sampler && sampler->sampling())
                _methodName = methodName;

    but the class definition for Sampler in a release build does not contain a declaration of a sampling() method.

2.) memoryProfilerInMMgc removes the last #endif for the repeat-header-inclusion guard:

    #endif //!__GCMemoryProfiler__

    (The patch as-is might still build, since I think there is a different #endif that was left in, but the point is that this seems like it may inject other problems.)

Anyway, I hope to finish the rebase soon.  I'll be keeping my updates in my clone of the mq repo:

  http://hg.mozilla.org/users/fklockii_adobe.com/redux-debugger-for-free/
(In reply to Felix S Klock II from comment #6)

The two-element list posted earlier was definitely not exhaustive.

I am not planning to enumerate all of the problems in the queue.

My short term plan is just to ensure primarily that every point along the queue is buildable, and then secondarily try to preserve the intention of the patch.  After that's done, I'll go back in and try to make sense out of the overall patch structure.
Finished rebasing at TR 7184:0d46338d1354; debug/release all build.

Pushed to:
  http://hg.mozilla.org/users/fklockii_adobe.com/redux-debugger-for-free/rev/09b5b152d987
(In reply to Felix S Klock II from comment #7)
> (In reply to Felix S Klock II from comment #6)
> 
> The two-element list posted earlier was definitely not exhaustive.
> 
> I am not planning to enumerate all of the problems in the queue.
> 
> My short term plan is just to ensure primarily that every point along the
> queue is buildable, and then secondarily try to preserve the intention of
> the patch.

Note that in some cases my work-arounds to return to a buildable state explicitly "moved backwards", in the sense that they would replace an #ifdef AVMPLUS_SAMPLER with an #ifdef DEBUGGER, which is the opposite of forward progress when one considers prior work item tickets like Bug 552297.
So, ugh, I'm pretty sure the *goal* of this ticket remains a good one.

The question is whether Lars's patch queue is worth trying to maintain/rebase/etc.

Anyway, I'm reassigning to Tony.

Tony: This might be a good task for Phil or for any other potential newcomer to the team.  Or you can always throw away the patch queue and start from scratch.  But you should consider at least looking over the patches for inspiration.
Assignee: fklockii → tony.printezis
You need to log in before you can comment on or make changes to this bug.