Bug 65422 – Simplify JSFunction creation for functions written in JS

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Simplify JSFunction creation for functions written in JS

[Oliver Hunt]

Created attachment 102447 [details]
Patch

[Darin Adler]

Comment on attachment 102447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102447&action=review

> Source/JavaScriptCore/runtime/Executable.cpp:147
> +    m_jsName.set(globalData, name.isEmpty() ? globalData.smallStrings.emptyString(&globalData) : jsString(&globalData, name.ustring()));

Can this be done with construction instead of assignment? Would that be a tiny bit better for performance?

This expression is long enough that it might be nice to share this code with an inline function instead of repeating it twice. The easiest way to do that would be an inline member function, or we could use an inline non-member function if we are doing it in construction.

Doesn’t jsString already handle the empty string case? Why do we need a special case for it here?

> Source/JavaScriptCore/runtime/Executable.h:481
> +        JSString* jsName() const { return m_jsName.get(); }

I would think of this as nameValue or wrappedName rather than jsName.

[Oliver Hunt]

Comment on attachment 102447 [details]
Patch

r- because i just copied the old logic (which did an empty string check) and i don't want to just blindly commit.

That said, we do have to do the setting of m_jsName late as jsString() may perform a gc allocation, and it is _never_ safe to do GC allocation in the initializer list (you may end up doing a gc sweep midway through executing your initializer list, and then you end up not marking yourself correctly :-( )

[Oliver Hunt]

Created attachment 102717 [details]
Patch

[Gavin Barraclough]

Comment on attachment 102717 [details]
Patch

You may want to rename jsName to nameValue per Darin's suggestion, and functionNameOffset() should probably return a size_t.  Otherwise all looks good.

[Oliver Hunt]

Created attachment 102720 [details]
Patch

[Oliver Hunt]

Committed r92250: <http://trac.webkit.org/changeset/92250>