Bug 115516 – Remove WTF::ListRefPtr class

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

ListRefPtr is used only by FontFamily class, besides it strongly depends on FontFamily class semantics which makes it non-generic and inappropriate for being present inside WTF.

[Mikhail Pozdnyakov]

Created attachment 200315 [details]
WIP

project files are not updated yet.

[Anders Carlsson]

Nice!

I seem to remember that ListRefPtr was added to avoid overflowing the stack when destroying AST Nodes in JSC when you had very deep syntax trees.

I'm not sure how many nested FontFamily objects you could have linked together, so it might be OK to just have the RefPtr and not do any special processing in the destructor.

[Mikhail Pozdnyakov]

(In reply to comment #2)
> Nice!
> 
> I seem to remember that ListRefPtr was added to avoid overflowing the stack when destroying AST Nodes in JSC when you had very deep syntax trees.
> 
> I'm not sure how many nested FontFamily objects you could have linked together, so it might be OK to just have the RefPtr and not do any special processing in the destructor.

I would keep the existing behaviour so far, as I am not sure that having just RefPtr won't bring the regression, because at least in principal it is still possible to face the same issue(overflowing the stack) again.

[Mikhail Pozdnyakov]

Created attachment 200392 [details]
patch

[Anders Carlsson]

Comment on attachment 200392 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200392&action=review

No need to update the order files, they're re-generated by someone in the WebKit performance team every once in a while.

> Source/WebCore/platform/graphics/FontFamily.h:74
> +    RefPtr<SharedFontFamily> reaper = m_next.release();
> +    while (reaper && reaper->hasOneRef())
> +        reaper = reaper->releaseNext(); // implicitly protects reaper->next, then derefs reaper

I'd add a comment stating that this initially came from ListRefPtr and that it can probably be a regular RefPtr.

[Mikhail Pozdnyakov]

Created attachment 200413 [details]
patch v2

Took comments from Anders into consideration.

[Mikhail Pozdnyakov]

Created attachment 200422 [details]
to be landed

[WebKit Commit Bot]

Comment on attachment 200422 [details]
to be landed

Clearing flags on attachment: 200422

Committed r149523: <http://trac.webkit.org/changeset/149523>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #0)
> it strongly depends on FontFamily class semantics

How does it depend on that?

[Mikhail Pozdnyakov]

(In reply to comment #10)
> (In reply to comment #0)
> > it strongly depends on FontFamily class semantics
> 
> How does it depend on that?

 ~ListRefPtr()
{
....
  reaper = reaper->releaseNext(); // implicitly protects reaper->next, then derefs reaper
}

[Darin Adler]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #0)
> > > it strongly depends on FontFamily class semantics
> > 
> > How does it depend on that?
> 
>  ~ListRefPtr()
> {
> ....
>   reaper = reaper->releaseNext(); // implicitly protects reaper->next, then derefs reaper
> }

Yes, that’s the requirement to be a list in the ListRefPtr sense--a releaseNext function. What was missing was documentation of that in the ListRefPtr header.

The reason I know this is not dependent on “FontFamily class semantics” is that it wasn’t even first created for FontFamily! It was created for JavaScriptCore. So I guess I am OK with removing this class, but I don’t think the remarks “strongly dependent on class semantics” and “non-generic” are fair.