ConstantStrings table should not be a GCRoot

RESOLVED FIXED in Q3 11 - Serrano

Status

P3
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: lhansen, Assigned: lhansen)

Tracking

unspecified
Q3 11 - Serrano
Bug Flags:
flashplayer-bug +

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

8 years ago
See Also: → bug 538545

Comment 1

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

Comment 2

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

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

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

Updated

8 years ago
Blocks: 576307
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → flash10.2
(Assignee)

Comment 5

8 years ago
Created attachment 475537 [details] [diff] [review]
Patch
Attachment #475537 - Flags: review?(treilly)

Comment 6

8 years ago
Comment on attachment 475537 [details] [diff] [review]
Patch

Why the explicit WB for _abcStrings?  Seems a DWB would be cleaner.  Otherwise looks good.
Attachment #475537 - Flags: review?(treilly) → review+
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

8 years ago
tamarin-redux-clean changeset:   5206:1d5a48477c0f

(with DWB fix and a comment about Stick() referencing the new bug #596529)
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Flags: flashplayer-bug+
You need to log in before you can comment on or make changes to this bug.