643371 - DataList<T> should use GC memory

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Lars T Hansen]

DataList uses mmfx memory, which requires all instances of these types to be finalized.  Finalization will probably become more expensive in the future (because it will be a special-purpose mechanism.)  We want to get rid of finalizers when we can.

There's no clear benefit to using mmfx allocation:

- The storage memory cannot be freed earlier than the holder of the memory, and garbage collection will reap both objects at the same time (if both are GC memory).

- It's true that mmfx memory does not have an on-page header but it's not clear what that benefit is.

- It may be a benefit for the blob memory not to be misidentified by conservative tracing, but we're moving toward exact tracing so it's not clear what this benefit will be.

Comment 1

[Steven Johnson]

So is the meta-bug here that Vector<> shouldn't use DataList?

Or that DataList (in general) shouldn't use mmfx?

Comment 2

[Lars T Hansen]

(In reply to comment #1)
> So is the meta-bug here that Vector<> shouldn't use DataList?
> 
> Or that DataList (in general) shouldn't use mmfx?

The former, because the latter seemed too risky to me, I had the impression that DataList could be used with no live GC.  But maybe not - if every DataList allocation requires a non-NULL live GC* parameter at present then it would not be risky.

On the topic of the former, it's possible that the same goes for ByteArray - it should use GC memory, not mmfx memory.  But I consider that a separate bug, to be tracked by a separate bugzilla.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #0)
> There's no clear benefit to using mmfx allocation:
> 
> - The storage memory cannot be freed earlier than the holder of the memory, and
> garbage collection will reap both objects at the same time (if both are GC
> memory).

I'm not an expert on this code but want to better understand the reasoning applied here, so I need a bit more elaboration: what about when the capacity has been exhausted and we need to replace the underlying storage?  That seems like a case where the storage memory would be freed earlier than the holder of the memory.

Comment 4

[Lars T Hansen]

(In reply to comment #3)
> (In reply to comment #0)
> > There's no clear benefit to using mmfx allocation:
> > 
> > - The storage memory cannot be freed earlier than the holder of the memory, and
> > garbage collection will reap both objects at the same time (if both are GC
> > memory).
> 
> I'm not an expert on this code but want to better understand the reasoning
> applied here, so I need a bit more elaboration: what about when the capacity
> has been exhausted and we need to replace the underlying storage?  That seems
> like a case where the storage memory would be freed earlier than the holder of
> the memory.

Of course, but we have GC::Free for that, to free a buffer, if we want the buffer to be GC memory.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #0)
> > > There's no clear benefit to using mmfx allocation:
> > > 
> > > - The storage memory cannot be freed earlier than the holder of the memory, and
> > > garbage collection will reap both objects at the same time (if both are GC
> > > memory).
> > 
> > I'm not an expert on this code but want to better understand the reasoning
> > applied here, so I need a bit more elaboration: what about when the capacity
> > has been exhausted and we need to replace the underlying storage?  That seems
> > like a case where the storage memory would be freed earlier than the holder of
> > the memory.
> 
> Of course, but we have GC::Free for that, to free a buffer, if we want the
> buffer to be GC memory.

Ah, so the point of your first bullet is to establish that using mmfx does not enable freeing the memory any sooner than the GC could, okay.  (That's assuming of course that the storage is not misidentified by conservative marking, as addressed by your third bullet.)  Thanks for the elaboration.

Comment 6

[Steven Johnson]

(In reply to comment #2)
> The former, because the latter seemed too risky to me, I had the impression
> that DataList could be used with no live GC.  But maybe not - if every DataList
> allocation requires a non-NULL live GC* parameter at present then it would not
> be risky.

Look at the code -- they all do indeed require a non-NULL GC, so they can advise about non-GC memory pressure. Historically it didn't on the theory there were use cases for it in non-GC environment, but we have transformed all those, so the only remaining argument was "reduce memory pressure", which may or may not be meaningful, your call.

Comment 7

[Tommy Reilly]

I think DataList should just use GC memory.

Comment 8

[Tommy Reilly]

Note that using GC memory doesn't mean finalization can go away completely, anything containing RCObject's still needs it.     Also DataList's being mmfx memory means they can be used in non-GCObjects however there are no cases of that in tamarin or the player, either the lists live in GCRoot's or the stack luckily.

Comment 9

[Tommy Reilly]

Attachment: use GC memory and assert List lives in scanned memory

Comment 10

[Steven Johnson]

Comment on attachment 547207 [details] [diff] [review]
use GC memory and assert List lives in scanned memory

Review of attachment 547207 [details] [diff] [review]:
-----------------------------------------------------------------

::: core/avmplusList.h
@@ +447,5 @@
>          void freeData(MMgc::GC* gc);
> +
> +        // update List pointer using a WriteBarrier if necessary.
> +        // This will assert if the address of m_data isn't GC or stack
> +        // memory (and it thus unreachable by the GC).  GCRoot's count

grammar nit: no apostrophe here, please, it's plural.

Comment 11

[Tamarin Bot]

changeset: 6480:017a48d16e22
user:      Tommy Reilly <[e-mail]>
summary:   Bug 643371 - DataList<T> should use GC memory (r=stejohns,sr-pending=edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/017a48d16e22

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

(this is in the process of being rolled back due to WE #2933522.  Patch will land in TR shortly.)

Comment 13

[Tamarin Bot]

changeset: 6515:a442c129794c
user:      Felix S Klock II <[e-mail]>
summary:   Bug 643371: Revert code to resolve WE #2933522 (r=rulohani).

http://hg.mozilla.org/tamarin-redux/rev/a442c129794c

Comment 14

[Tommy Reilly]

I think this should land but android crashes prevent it for now.   Another consideration is making the DataList's used by PoolObject use LeafVector instead of DataList.   That will at least make those things be GC memory and avoid unmanaged allocation overhead.

Comment 15

[Tommy Reilly]

Comment on attachment 547207 [details] [diff] [review]
use GC memory and assert List lives in scanned memory

Cancelling superreview until WE bug is resolved.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.