Bug 70364

Summary: Switched ropes from malloc memory to GC memory
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: New BugsAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch barraclough: review+

Description Geoffrey Garen 2011-10-18 14:23:29 PDT
Switched ropes from malloc memory to GC memory
Comment 1 Geoffrey Garen 2011-10-18 14:48:41 PDT
Created attachment 111507 [details]
Patch
Comment 2 Gavin Barraclough 2011-10-18 18:31:42 PDT
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)?
Comment 3 Geoffrey Garen 2011-10-18 19:02:03 PDT
> > 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.
Comment 4 Geoffrey Garen 2011-10-18 19:53:28 PDT
Committed r97827: <http://trac.webkit.org/changeset/97827>