Bug 27831 - Allow custom memory allocation control for JavaScriptCore's UString class
Summary: Allow custom memory allocation control for JavaScriptCore's UString class
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-30 05:18 PDT by Zoltan Horvath
Modified: 2011-06-17 01:05 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch (1.09 KB, patch)
2009-07-30 05:21 PDT, Zoltan Horvath
eric: review-
Details | Formatted Diff | Diff
updated patch (1.19 KB, patch)
2010-01-18 01:53 PST, Zoltan Horvath
aroben: review-
Details | Formatted Diff | Diff
updated proposed patch (1.15 KB, patch)
2010-01-18 09:58 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2009-07-30 05:18:35 PDT
Inherits UString class from FastAllocBase because it has been
instantiated by 'new' in JavaScriptCore/runtime/UString.cpp:212.
Comment 1 Zoltan Horvath 2009-07-30 05:21:54 PDT
Created attachment 33770 [details]
proposed patch
Comment 2 Eric Seidel (no email) 2009-07-31 15:09:34 PDT
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
Comment 3 Zoltan Horvath 2009-09-02 00:14:51 PDT
This patch breaks the layout tests on mac me too. I mark it as invalid.
Comment 4 Zoltan Horvath 2010-01-18 01:50:15 PST
I tested it again,  now the layout tests work well.
Comment 5 Zoltan Horvath 2010-01-18 01:53:49 PST
Created attachment 46805 [details]
updated patch
Comment 6 Adam Roben (:aroben) 2010-01-18 07:44:14 PST
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?
Comment 7 Zoltan Horvath 2010-01-18 09:58:14 PST
Created attachment 46831 [details]
updated proposed patch

I just uploaded from wrong directory. This is the good one.
Comment 8 WebKit Commit Bot 2010-01-18 16:51:04 PST
Comment on attachment 46831 [details]
updated proposed patch

Clearing flags on attachment: 46831

Committed r53438: <http://trac.webkit.org/changeset/53438>
Comment 9 WebKit Commit Bot 2010-01-18 16:51:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2010-01-18 17:17:51 PST
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.
Comment 11 Darin Adler 2010-01-18 17:20:09 PST
Rolled out in <http://trac.webkit.org/changeset/53438>.
Comment 12 Darin Adler 2010-01-18 17:26:55 PST
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.
Comment 13 Darin Adler 2010-01-18 17:35:32 PST
I believe the build was broken only on 64-bit platforms, so the SnowLeopard bot showed the problem but the Leopard one did not.
Comment 14 Eric Seidel (no email) 2010-01-18 17:58:21 PST
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. :(
Comment 15 Zoltan Horvath 2010-01-20 06:13:47 PST
(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.
Comment 16 Darin Adler 2010-01-20 07:51:01 PST
(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.
Comment 17 Gavin Barraclough 2011-06-16 21:52:44 PDT
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.
Comment 18 Zoltan Horvath 2011-06-17 01:05:42 PDT
(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>.