501619 - Smart pointers need copy constructrors

Status: VERIFIED FIXED

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

Description

[Lars T Hansen]

DRC(Stringp) is just MMgc::RCPtr<Stringp>; that object does not have a copy constructor.  But in the AvmCore constructor, we have the assignment

  booleanStrings[0] = kfalse;

where both sides are preexisting DRC(Stringp) types.  Compilers generate bitwise copies here, but that's wrong - reference counts must be adjusted.  So a copy constructor is needed.

The smart pointer class does have an assignment operator but it's not called here, because its argument type is Stringp, not DRC(Stringp).  That could be another bug.

We need to inspect all the smart pointer classes for this sort of bug.

Comment 1

[Lars T Hansen]

There are only those two problems in the VM code for RCPtr.

ZeroPtr has the same issue but there are no problems in the code for ZeroPtr.

The two write barrier smart pointer classes are already OK; they have a proper assignment operator and a hidden copy constructor.

Comment 2

[Lars T Hansen]

Attachment: Patch v1 (untested)

My source tree is in a shambles right now so I haven't tested this, but it compiles and the generated code is different.

Comment 3

[Lars T Hansen]

Attachment: Patch v2

Comment 4

[Michael Daumling]

Comment on attachment 386245 [details] [diff] [review]
Patch v2

Is it not always a good idea to increment the ref before decrementing it? If you assign the same string, you could push it to the ZCT and remove it again, causing unneccessary overhead. I would other.t->IncrementRef() before DecrementRef() - just in case.

Comment 5

[Michael Daumling]

Attachment: Suggested patch with IncrementRef() first

This is just a suggestion. Please decide yourself.

Comment 6

[Lars T Hansen]

(In reply to comment #4)
> (From update of attachment 386245 [details] [diff] [review])
> Is it not always a good idea to increment the ref before decrementing it? If
> you assign the same string, you could push it to the ZCT and remove it again,
> causing unneccessary overhead.

This is true.

On the other hand, if you Increment first and Decrement later you may overflow the reference count field, making the object sticky :-)

I'm guessing the likelihoods of the two events aren't that diffent and that in either case neither really creates a problem worth optimizing for.  I did it the way I did it because that's the order used everywhere else in MMgc, so we'll stick with it for now.

Comment 7

[Lars T Hansen]

(In reply to comment #3)
> Created an attachment (id=386245) [details]
> Patch v2

redux changeset:   2081:fe1863fe4e68

Comment 8

[Steven Johnson]

(In reply to comment #6)
> I'm guessing the likelihoods of the two events aren't that diffent and that in
> either case neither really creates a problem worth optimizing for.  I did it
> the way I did it because that's the order used everywhere else in MMgc, so
> we'll stick with it for now.

I beg to differ: self-assignment can easily happen, and while premature stickiness is bad, premature freeing is much worse :-)

that said, there's an easy workaround: test for newval != oldval first, which gives us the best of both.

Comment 9

[Lars T Hansen]

There's no chance of premature freeing because the object is inserted into the ZCT when the reference count drops to zero and removed from the ZCT when the reference count raises above zero.  The only way the object could be freed prematurely is if the ZCT is reaped between the decrement and the increment, but that can't happen in MMgc. (The ZCT reaper can be triggered if Add overflows the ZCT, but at that point the value to insert is either in the ZCT already or on the stack and won't be deleted.)  So this is only a question about efficiency.

Re adding a test for newVal != oldVal, I think you have to convince me that the frequency of self-assignment is such that the time spent in the reference counting for those assignments outweighs the time spent on the test for all other assignments, on balance, across all programs.  I doubt it's the case (but I really don't know for sure).

Comment 10

[Steven Johnson]

ah -- you're right, I forgot about ZCT. (I was thinking COM-style refcounting where release-to-zero frees immediately.)

Comment 11

[Michael Daumling]

There *is* overhead involved with the adding to and the removal from the ZCT, as I mentioned above. the StringObject class has these this == other tests in place, since comments suggested that this case was hit quite frequently.

IMHO a newVal != oldVal comparison should at least be added, and if that is too much overhead, well, just use my suggested patch.