592843 - Remove mmgc.supp and use client requests

Status: RESOLVED FIXED

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

Description

[Tommy Reilly]

This will take a couple refactorings and patches to get right, worth a dedicated bug

Comment 1

[Tommy Reilly]

Attachment: Replace linked list code with helper object

This consolidates a lot of VALGRIND client requests into one place.  Thoughts?   Concerns about landing?   The generated assembly code for Release builds is no different than before where it counts except for some reason a little more stack space is needed (in particular GCAlloc::Alloc went from 0x20 to 0x30 bytes gcc, mac 4.2).

Comment 2

[Lars T Hansen]

Comment on attachment 471277 [details] [diff] [review]
Replace linked list code with helper object

Personally I'm pretty averse to this style of programming (iterators) and in particular to non-functional "list" structures - ECMAScript Arrays have scarred me for life.  Tastes differ though.

There's no file here with the LinkedList implementation itself.  Calling it LinkedList may be a bit too much, if it threads through the first (?) word in the object it stores.  I never see it used with anything but void*, does it need to be a templated type?

More stack space may be needed because variables are not properly enregistered in iterations, I would guess, or perhaps likely, the iterator needs multiple values stored next to each other.  (Speculation obviously.)

What's the motivation behind this change?  All the Valgrind calls in this patch don't touch the lists at all.

In summary, yes, I have concerns about landing this.  But we should talk more.

Comment 3

[Tommy Reilly]

Attachment: Re-attach with LinkedList impl this time

Comment 4

[Tommy Reilly]

Attachment: Different approach

I like this approach better, whaddya think? Where should I put FL* functions?

Comment 5

[Tommy Reilly]

Attachment: Use simple helpers to consolidate freelist code

Comment 6

[Lars T Hansen]

Comment on attachment 472626 [details] [diff] [review]
Use simple helpers to consolidate freelist code

The abstraction is nice.

I don't understand why FLSeed is called that.  FLCarveOffBlock or somesuch seems more descriptive (open to suggestions).

I dislike that utilities defined in FixedAlloc code is used in GC code; a separate header may seem like overkill but it would still be cleaner (if we can't find a better place).

FLPopAndZero is used just once, and in a situation where the old code did not in fact clear the first word (ie FLPop was being used).  Motivation?

Comment 7

[Tommy Reilly]

(In reply to comment #6)
> I don't understand why FLSeed is called that.  FLCarveOffBlock or somesuch
> seems more descriptive (open to suggestions).

Cause we're seeding the freelist with its initial values?  How about FLWrite?

> I dislike that utilities defined in FixedAlloc code is used in GC code; a
> separate header may seem like overkill but it would still be cleaner (if we
> can't find a better place).

Agreed

> FLPopAndZero is used just once, and in a situation where the old code did not
> in fact clear the first word (ie FLPop was being used).  Motivation?

The old code had this but it was a couple lines later:

        // Clear free header, the rest was cleared in Free()
        p[0] = 0;

Comment 8

[Lars T Hansen]

(In reply to comment #7)
> (In reply to comment #6)
> > I don't understand why FLSeed is called that.  FLCarveOffBlock or somesuch
> > seems more descriptive (open to suggestions).
> 
> Cause we're seeding the freelist with its initial values?

Aha.

> How about FLWrite?

Sounds too much like FLPush.  How about FLAllocateAndPush?

> > I dislike that utilities defined in FixedAlloc code is used in GC code; a
> > separate header may seem like overkill but it would still be cleaner (if we
> > can't find a better place).
> 
> Agreed

OK, I'm assuming you'll come up with something snazzy here.

> > FLPopAndZero is used just once, and in a situation where the old code did not
> > in fact clear the first word (ie FLPop was being used).  Motivation?
> 
> The old code had this but it was a couple lines later:
> 
>         // Clear free header, the rest was cleared in Free()
>         p[0] = 0;

Gotcha.

Comment 9

[Tommy Reilly]

FL funcs:

http://hg.mozilla.org/users/treilly_adobe.com/tr-valgrind/rev/2e8fc8a6f164
http://hg.mozilla.org/users/treilly_adobe.com/tr-valgrind/rev/41c66457bc69