If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

DataList<T> should use GC memory

NEW
Assigned to

Status

Tamarin
Virtual Machine
P3
normal
7 years ago
5 years ago

People

(Reporter: Lars T Hansen, Assigned: Tommy Reilly)

Tracking

(Blocks: 3 bugs)

unspecified
Future
Dependency tree / graph
Bug Flags:
flashplayer-qrb +

Details

(Whiteboard: WE:2933522, loose-end)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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

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

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

Comment 2

7 years ago
(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.
(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.
(Reporter)

Comment 4

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

7 years ago
(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.
(Reporter)

Updated

6 years ago
Assignee: lhansen → rulohani

Updated

6 years ago
Assignee: rulohani → nobody
(Assignee)

Comment 7

6 years ago
I think DataList should just use GC memory.
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → treilly
Blocks: 654945
(Assignee)

Updated

6 years ago
Summary: Vector.<int>, Vector.<uint>, and Vector.<Number> should probably not use DataList → DataList<T> should use GC memory
(Assignee)

Comment 9

6 years ago
Created attachment 547207 [details] [diff] [review]
use GC memory and assert List lives in scanned memory
Attachment #547207 - Flags: review?(stejohns)
(Assignee)

Updated

6 years ago
Attachment #547207 - Attachment description: use GC memory and List lives in scanned memory → use GC memory and assert List lives in scanned memory

Comment 10

6 years ago
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.
Attachment #547207 - Flags: review?(stejohns) → review+
(Assignee)

Updated

6 years ago
Attachment #547207 - Flags: superreview?(edwsmith)

Comment 11

6 years ago
changeset: 6480:017a48d16e22
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 643371 - DataList<T> should use GC memory (r=stejohns,sr-pending=edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/017a48d16e22
(this is in the process of being rolled back due to WE #2933522.  Patch will land in TR shortly.)
Whiteboard: WE:2933522

Comment 13

6 years ago
changeset: 6515:a442c129794c
user:      Felix S Klock II <fklockii@adobe.com>
summary:   Bug 643371: Revert code to resolve WE #2933522 (r=rulohani).

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

Updated

6 years ago
Flags: flashplayer-qrb+
(Assignee)

Updated

6 years ago
Target Milestone: Q1 12 - Brannan → Future
(Assignee)

Comment 14

6 years ago
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.
(Assignee)

Comment 15

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

Cancelling superreview until WE bug is resolved.
Attachment #547207 - Flags: superreview?(edwsmith)
Status: ASSIGNED → NEW
Whiteboard: WE:2933522 → WE:2933522, loose-end
You need to log in before you can comment on or make changes to this bug.