| Summary: | Allow custom memory allocation control for JavaScriptCore's UString class | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zoltan Horvath <zoltan> | ||||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED INVALID | ||||||||||
| Severity: | Normal | CC: | andersca, barraclough, commit-queue, darin, eric, ggaren, mjs | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Zoltan Horvath
2009-07-30 05:18:35 PDT
Created attachment 33770 [details]
proposed patch
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
This patch breaks the layout tests on mac me too. I mark it as invalid. I tested it again, now the layout tests work well. Created attachment 46805 [details]
updated patch
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?
Created attachment 46831 [details]
updated proposed patch
I just uploaded from wrong directory. This is the good one.
Comment on attachment 46831 [details] updated proposed patch Clearing flags on attachment: 46831 Committed r53438: <http://trac.webkit.org/changeset/53438> All reviewed patches have been landed. Closing bug. 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. Rolled out in <http://trac.webkit.org/changeset/53438>. 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. I believe the build was broken only on 64-bit platforms, so the SnowLeopard bot showed the problem but the Leopard one did not. 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. :( (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. (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. 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. (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>. |