WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED INVALID
27831
Allow custom memory allocation control for JavaScriptCore's UString class
https://bugs.webkit.org/show_bug.cgi?id=27831
Summary
Allow custom memory allocation control for JavaScriptCore's UString class
Zoltan Horvath
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Zoltan Horvath
Comment 1
2009-07-30 05:21:54 PDT
Created
attachment 33770
[details]
proposed patch
Eric Seidel (no email)
Comment 2
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
Zoltan Horvath
Comment 3
2009-09-02 00:14:51 PDT
This patch breaks the layout tests on mac me too. I mark it as invalid.
Zoltan Horvath
Comment 4
2010-01-18 01:50:15 PST
I tested it again, now the layout tests work well.
Zoltan Horvath
Comment 5
2010-01-18 01:53:49 PST
Created
attachment 46805
[details]
updated patch
Adam Roben (:aroben)
Comment 6
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?
Zoltan Horvath
Comment 7
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.
WebKit Commit Bot
Comment 8
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
>
WebKit Commit Bot
Comment 9
2010-01-18 16:51:13 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 10
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.
Darin Adler
Comment 11
2010-01-18 17:20:09 PST
Rolled out in <
http://trac.webkit.org/changeset/53438
>.
Darin Adler
Comment 12
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.
Darin Adler
Comment 13
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.
Eric Seidel (no email)
Comment 14
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. :(
Zoltan Horvath
Comment 15
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
.
Darin Adler
Comment 16
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.
Gavin Barraclough
Comment 17
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.
Zoltan Horvath
Comment 18
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
>.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug