Bug 34864 – Many objects left uncollected after visiting mail.google.com and closing window

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Many objects left uncollected after visiting mail.google.com and closing window

[Geoffrey Garen]

Created attachment 48594 [details]
Patch

[Geoffrey Garen]

Committed r54696: <http://trac.webkit.org/changeset/54696>

[Geoffrey Garen]

Created attachment 48619 [details]
Patch

[Geoffrey Garen]

Reopening to post the second half of this fix.

[Oliver Hunt]

Comment on attachment 48619 [details]
Patch

r=me

[Darin Adler]

Comment on attachment 48619 [details]
Patch

> +        Don't unconditionally hang onto small strings. Instead, hang onto all
> +        small strings as long as any small string is still referenced.

What is the reasoning for that rule? I'm sure it's a good idea, but I think SmallStrings::markChildren should have a "why" comment in it explaining why it is.

> +        if (isMarked(m_singleCharacterStrings[i]))
> +            isAnyStringMarked = true;

I suggest just doing assignment here instead of an if statement to save a branch.

[Geoffrey Garen]

Committed r54701: <http://trac.webkit.org/changeset/54701>

[Geoffrey Garen]

> What is the reasoning for that rule? I'm sure it's a good idea, but I think
> SmallStrings::markChildren should have a "why" comment in it explaining why it
> is.

Added this comment:

    /*
       Our hypothesis is that small strings are very common. So, we cache them
       to avoid GC churn. However, in cases where this hypothesis turns out to
       be false -- including the degenerate case where all JavaScript execution
       has terminated -- we don't want to waste memory.

       To test our hypothesis, we check if any small string has been marked. If
       so, it's probably reasonable to mark the rest. If not, we clear the cache.
     */

> > +        if (isMarked(m_singleCharacterStrings[i]))
> > +            isAnyStringMarked = true;
> 
> I suggest just doing assignment here instead of an if statement to save a
> branch.

Fixed.