596207 - ConstantStrings table should not be a GCRoot

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

There are several reasons why it's not ideal for this table to be a GCRoot:

- it's scanned twice during every GC (that's how roots are)
- it adds to the volume of roots, which increases the risk of creating
  noticeable GC pauses (the root set is scanned atomically, though large
  roots are splits)
- roots are always scanned conservatively, which makes them more costly,
  even if in this case the object is dense with pointers

If the ConstantStrings table were a regular GC object then we'd need to use barriers to write it when it's created, but that seems like a small cost to pay to avoid having to scan it twice during every GC for the lifetime of the program.  (That will be especially true once the write barrier is cheaper).

Comment 1

[Tommy Reilly]

+1 might be overkill but a permanent generation would be good for this.    A sticky RCObject is pretty much const so that might be a way to make these Strings come from non-GC memory, we'd get in trouble if GetGC() was used on them though.   If we got that right though the Strings could be shared across vms/threads.

Comment 2

[Lars T Hansen]

Agree that not scanning these on every GC would be a win, though my inclination would be to let the generational GC deal with that as it deals with all data - once we have a permanent region we need to worry about how that interacts with unloading, and the generational GC will need to have a general mechanism for that anyway.

Since you bring it up and I'd like to know anyway, can you tell me why the constant strings are made sticky in PoolObject::getString?  Workaround for some bug?

Comment 3

[Tommy Reilly]

(In reply to comment #2) 
> Since you bring it up and I'd like to know anyway, can you tell me why the
> constant strings are made sticky in PoolObject::getString?  Workaround for some
> bug?

The JIT buries constant pool Stringp's in the code (or at least it used to).

Comment 4

[Edwin Smith]

Every pointer the JIT embeds is a copy of a pointer from an object that is guaranteed to live longer than the code, usually PoolObject, so it is not justification for making an RC pointer be Sticky.  The Sticky() call in PoolObject::getString() was added by this patch:

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

Comment 5

[Lars T Hansen]

Attachment: Patch

Comment 6

[Tommy Reilly]

Comment on attachment 475537 [details] [diff] [review]
Patch

Why the explicit WB for _abcStrings?  Seems a DWB would be cleaner.  Otherwise looks good.

Comment 7

[Lars T Hansen]

(In reply to comment #6)
> Comment on attachment 475537 [details] [diff] [review]
> Patch
> 
> Why the explicit WB for _abcStrings?  Seems a DWB would be cleaner.  Otherwise
> looks good.

It's an accident of the history of the patch, and I just left it in.  I'll fix it before pushing.

Comment 8

[Lars T Hansen]

(In reply to comment #4)
> Every pointer the JIT embeds is a copy of a pointer from an object that is
> guaranteed to live longer than the code, usually PoolObject, so it is not
> justification for making an RC pointer be Sticky.  The Sticky() call in
> PoolObject::getString() was added by this patch:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=387632&action=diff

Right.  Stick() does not protect anything against garbage collection, only against deletion by reference counting.  If it is as you say that the jitted code is never the longest-lived reference to the string object then the Stick() call, if it turns out to be necessary, probably masks an RC bug elsewhere.

I'll open a separate bug for that.

Comment 9

[Lars T Hansen]

tamarin-redux-clean changeset:   5206:1d5a48477c0f

(with DWB fix and a comment about Stick() referencing the new bug #596529)