Summary: | Switched ropes from malloc memory to GC memory | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||
Component: | New Bugs | Assignee: | Geoffrey Garen <ggaren> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | barraclough | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Geoffrey Garen
2011-10-18 14:23:29 PDT
Created attachment 111507 [details]
Patch
Comment on attachment 111507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111507&action=review Hmmm, I wonder whether we still need m_fiberCount, if a JSString is a rope, is m_value null? (to remove the test of m_fiberCount in isRope) Still, that's a separate question really. After this patch, will we ever be creating RopeImpls? Should this patch not be removing RopeImpl.cpp/.h from svn? r+, but I think you should probably be using jsNontrivialString in the case I point out, and probably remove the RopeImpl files. > Source/JavaScriptCore/runtime/JSValue.cpp:222 > + return JSString::create(exec->globalData(), UString("undefined").impl()); Maybe we should have an JSString::create(ExecState*, const char*) helper? - Actually, I think we have one! - this should call: inline JSString* jsNontrivialString(JSGlobalData* globalData, const char* s) > Source/JavaScriptCore/runtime/Operations.h:96 > ALWAYS_INLINE JSValue jsString(ExecState* exec, JSValue thisValue) This method is really poorly named (my bad, I believe :-( ) - there is nothing in the name indicating it does anything to do with arguments, we should really rename it to something more explicit. > Source/JavaScriptCore/runtime/SmallStrings.cpp:109 > + m_emptyString = JSString::createHasOtherOwner(*globalData, UString("").impl()); This could be 'StringImpl::empty()' > Source/JavaScriptCore/runtime/StringPrototype.cpp:640 > + : jsString(exec, asString(thisValue), JSString::create(exec->globalData(), v.toString(exec).impl()))); We create JSStrings with arguments like this in quite a few places, maybe we should have an JSString::create(ExecState*, JSValue)? > > Source/JavaScriptCore/runtime/JSValue.cpp:222
> > + return JSString::create(exec->globalData(), UString("undefined").impl());
>
> Maybe we should have an JSString::create(ExecState*, const char*) helper? -
> Actually, I think we have one! - this should call:
>
> inline JSString* jsNontrivialString(JSGlobalData* globalData, const char* s)
Ah yes. Actually, all of my direct calls to JSString::create are broken on Windows, since they miss the glorious call to fixupVPtr. I'll fix those, and fix the rest in a follow-up patch.
Committed r97827: <http://trac.webkit.org/changeset/97827> |