596918 - Stick() call on constant strings is either redundant or masks a bug elsewhere

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

(The background is bug #596207.)

In PoolObject::getString every new String object has its reference count pinned at the maximum value through the use of RCObject::Stick() before being placed in the pool.  Nobody understands any longer why that is.

The call made an appearance with this changeset:

  https://bugzilla.mozilla.org/attachment.cgi?id=387632&action=diff

It has been theorized that the Stick call is necessary because the JIT embeds pointers to the strings without incrementing the string's reference count.  Thus if jitted code has the last reference(s) to a string then the string would be reaped prematurely.  However, that does not make sense on its own because garbage collection also would not see string pointers from jitted code and would take the string anyway; Stick would not protect the string from being deleted by GC.

It is thus likely that the Stick call is either redundant, or it masks an RC / reachability bug elsewhere in the code.  That should be investigated.

Comment 1

[Edwin Smith]

I don't know what posessed me to re-read the patch just now, when I read it once, but I found this comment in the deleted code that used to eagerly initialize the string constant pool.  (getString() replaced it, doing lazy init)

Near PoolObject.cpp:1106

    // Jit skips WB on string constants so make them sticky
    // fixme -- it's incorrect to skip WB's on const's, bug was fixed;
    // is this still required?
    s->Stick();

This implies the "// must be made sticky for now..." comment in the new refactored getString() was either discovered the hard way, or placed there defensively.

I remember the missing-WB-on-string-constants bug, IIRC Werner fixed it.  In any case, the JIT no longer skips WB's on RCPointer* slots, for any kind of value.  (It cant!  it must decrement the refcount for whatever was there previously).

Comment 2

[Lars T Hansen]

If it masks a bug elsewhere then we're probably going to have to track down that bug before we remove reference counting.

Comment 3

[Werner Sharp]

FWIW, after removing the Stick, the TR acceptance suite and the player's ATS9 and ATS10 all pass.  

But related to bug 605202, knowing a string is sticky would allow us to skip half of a privateWriteBarrierRC which may help performance.

Comment 4

[Werner Sharp]

Original CL in the player that added the Stick() call was 192575 from treilly on 12/2/2005.

Severity: severe
Summary: integrate new DRC
Dev/QA impact: watch out for new bugs ;-)
Smokes passed: ATS8, ATS8+, flexstore, sanities

Details:

DRC now scans the stack and runs whenever the ZCT is full, used to be
we ran only at AVM- AS execution edges and resorted to a full collection
when it got full.  We can do this b/c we now scan the stack for RCObjects
and don't let them be reaped.

A side affect is that dirty stack references pins objects so now all ScriptObject*'s
are ScriptObjectp and this is typedefs to ZeroPtr<ScriptObject*> for Release builds
(debug builds clean the stack already and the ZeroPtr hurts performance when it 
isn't optimized).

There will be more details for dev's on what they need to know up on the wiki soon,
stay tuned.

Comment 5

[Tommy Reilly]

conjecture: I bet if we remove the if(Sticky()) checks from IncrementRef/DecrementRef the speedup will beat the speedup from leaving in Stick() on cpool strings and skipping the IncrementRef.

Comment 6

[Werner Sharp]

Sure but you can't do that for all objects, right?  Something can be assigned to 256 RC slots and become Sticky?

Comment 7

[Tommy Reilly]

We can remove the STICKY flag and just have one test for refcount == RCBITS instead.

Comment 8

[Werner Sharp]

Post IM discussion with Tommy: Why does Stick() not just set the RCBITS to 255 and we use that as the sticky flag?  Why do we need a separate bit?   Switching this would save one check in IncrementRef where we set the STICKYFLAG:

            if((composite&RCBITS) == RCBITS) {
                composite |= STICKYFLAG;

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> Post IM discussion with Tommy: Why does Stick() not just set the RCBITS to 255
> and we use that as the sticky flag?  Why do we need a separate bit?   Switching
> this would save one check in IncrementRef where we set the STICKYFLAG:
> 
>             if((composite&RCBITS) == RCBITS) {
>                 composite |= STICKYFLAG;

Interesting point, may be worth experimenting with for Serrano.  I will log a separate bug for it.

Comment 10

[Lars T Hansen]

AbcParser::parseMetadataInfos() looks dodgy.  In the class, we have

 Stringp* metaNames;

and then in parseMetadataInfos we have

 metaNames =
   (Stringp*) core->GetGC()->Calloc(metadataCount, 
                                    sizeof(Stringp),
                                    MMgc::GC::kContainsPointers);
 for (uint32_t i=0; i < metadataCount; i++)
 {
   ...
   uint32_t index = readU30(pos);
   Stringp name = resolveUtf8(index);
   // constant pool names are stuck and always reachable via PoolObject,
   // DRC or WB
   metaNames[i] = name;
   ...

Note the absence of a barrier justified by the comment that the names are "stuck".  Then, look at resolveUtf8:

 Stringp AbcParser::resolveUtf8(uint32_t index) const
 {
   if (index > 0 && index < pool->constantStringCount)
     return pool->getString(index);
   ...

and furthermore at getString:

 Stringp PoolObject::getString(int32_t index) const
 {
   ConstantStringData* dataP = _abcStrings + index;
   if (dataP->abcPtr >= _abcStringStart && dataP->abcPtr < _abcStringEnd)
   {
     uint32_t len = AvmCore::readU32(dataP->abcPtr);
     Stringp s = core->internStringUTF8((const char*) dataP->abcPtr,
                                        len,
                                        true,
                                        false);
     s->Stick();
     ...

Clearly internStringUTF8() can allocate - the first thing it does is call String::createUTF8(), and it can also rehash the intern table - ergo a barrier is missing on the assignment to metaNames unless the string is actually stuck in getString().

Comment 11

[Lars T Hansen]

Metanames was dealt with in bug #615543.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.