Bug 27831 – Allow custom memory allocation control for JavaScriptCore's UString class

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Zoltan Horvath]

Inherits UString class from FastAllocBase because it has been
instantiated by 'new' in JavaScriptCore/runtime/UString.cpp:212.

[Zoltan Horvath]

Created attachment 33770 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 33770 [details]
proposed patch

Did you run the layout tests?
11027 test cases (99%) succeeded
1 test case (<1%) timed out
5 test cases (<1%) crashed
14 test cases (<1%) had stderr output

[Zoltan Horvath]

This patch breaks the layout tests on mac me too. I mark it as invalid.

[Zoltan Horvath]

I tested it again,  now the layout tests work well.

[Zoltan Horvath]

Created attachment 46805 [details]
updated patch

[Adam Roben (:aroben)]

Comment on attachment 46805 [details]
updated patch

This patch changes JSC::CString, not JSC::UString as the bug and ChangeLog suggest. What's really going on here?

[Zoltan Horvath]

Created attachment 46831 [details]
updated proposed patch

I just uploaded from wrong directory. This is the good one.

[WebKit Commit Bot]

Comment on attachment 46831 [details]
updated proposed patch

Clearing flags on attachment: 46831

Committed r53438: <http://trac.webkit.org/changeset/53438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

I have bad news.

First, I have to roll this out because adding FastAllocBase as a base class made UString bigger than a single pointer, and in turn that made JSString too big for a garbage collection cell.

Second, that means that it’s likely that adding FastAllocBase as base class for other objects has been making them all larger, inflating WebKit’s memory use.

Someone needs to investigate this further.

[Darin Adler]

Rolled out in <http://trac.webkit.org/changeset/53438>.

[Darin Adler]

Anders Carlsson explained to me what’s going wrong.

The problem is that RefPtr derives from FastAllocBase and so does UString. Since the first member of UString is a RefPtr, it can’t be at the same address of the FastAllocBase we are inheriting from. So padding is added, making the object bigger.

This will happen any time the first data member of a class is an object of a class that derives from FastAllocBase (or Noncopyable or anything else that eventually leads up to FastAllocBase), and the class itself also derives from FastAllocBase (or Noncopyable, etc.).

One way to eliminate the problem is to make FastAllocBase a class template and inherit from FastAllocBase<X> so that each use of FastAllocBase is a unique class, but this still won’t help in the case of Noncopyable.

We may want to file a new bug about this. FastAllocBase is probably making many of our objects a bit bigger.

[Darin Adler]

I believe the build was broken only on 64-bit platforms, so the SnowLeopard bot showed the problem but the Leopard one did not.

[Eric Seidel (no email)]

The commit-queue really needs to be taught how to roll out its own patches whne it turns the bots red.  Sorry about the break. :(

[Zoltan Horvath]

(In reply to comment #10)
> First, I have to roll this out because adding FastAllocBase as a base class
> made UString bigger than a single pointer, and in turn that made JSString too
> big for a garbage collection cell.

I didn't know that UString is a GC collected class . Where is it converted to JSString?

I opened a new bug "FastAllocBase is probably making many of our objects a bit bigger" bug #33896.

[Darin Adler]

(In reply to comment #15)
> I didn't know that UString is a GC collected class . Where is it converted to
> JSString?

It's not a GC-collected class. But JSString objects have a UString as a data member. You can find it in JSString.h.

[Gavin Barraclough]

We no longer use FastAllocBase, and we do not heap allocate individual UString objects (it is just a pointer).  This patch does not seem valid.

[Zoltan Horvath]

(In reply to comment #17)
> We no longer use FastAllocBase, and we do not heap allocate individual UString objects (it is just a pointer).  This patch does not seem valid.

Yes, exactly! It has been rolled out in <http://trac.webkit.org/changeset/53438>.