Bug 95633 – Improve JSC use of Strings after the UString->String change

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Benjamin Poulain]

Take advantage of the goodness of String.

Some microbenchmark improved: http://jsperf.com/js-error-bench

[Benjamin Poulain]

Created attachment 161815 [details]
Patch

[Benjamin Poulain]

Created attachment 161817 [details]
Patch

[Build Bot]

Comment on attachment 161817 [details]
Patch

Attachment 161817 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13724456

[Geoffrey Garen]

Comment on attachment 161817 [details]
Patch

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

This patch looks good, but I'm wondering about these null -> empty string changes.

> Source/JavaScriptCore/runtime/Error.h:92
> -            function->finishCreation(exec->globalData(), "");
> +            function->finishCreation(exec->globalData(), String());

Why is this change from empty string to null string OK?

> Source/JavaScriptCore/runtime/FunctionPrototype.h:35
> -            prototype->finishCreation(exec, "");
> +            prototype->finishCreation(exec, String());

Why is this change from empty string to null string OK?

> Source/JavaScriptCore/runtime/InternalFunction.cpp:47
> -    putDirect(globalData, globalData.propertyNames->name, jsString(&globalData, name.isNull() ? "" : name), DontDelete | ReadOnly | DontEnum);
> +    putDirect(globalData, globalData.propertyNames->name, jsString(&globalData, name), DontDelete | ReadOnly | DontEnum);

Why is this change from empty string to null string OK?

> Source/JavaScriptCore/runtime/JSFunction.cpp:99
> -    putDirect(exec->globalData(), exec->globalData().propertyNames->name, jsString(exec, name.isNull() ? "" : name), DontDelete | ReadOnly | DontEnum);
> +    putDirect(exec->globalData(), exec->globalData().propertyNames->name, jsString(exec, name), DontDelete | ReadOnly | DontEnum);

Why is this change from empty string to null string OK?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:210
> -    protoAccessor->setGetter(exec->globalData(), JSFunction::create(exec, this, 0, "", globalFuncProtoGetter));
> -    protoAccessor->setSetter(exec->globalData(), JSFunction::create(exec, this, 0, "", globalFuncProtoSetter));
> +    protoAccessor->setGetter(exec->globalData(), JSFunction::create(exec, this, 0, String(), globalFuncProtoGetter));
> +    protoAccessor->setSetter(exec->globalData(), JSFunction::create(exec, this, 0, String(), globalFuncProtoSetter));

Why is this change from empty string to null string OK?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:329
> -    JSFunction* thrower = JSFunction::create(exec, this, 0, "", globalFuncThrowTypeError);
> +    JSFunction* thrower = JSFunction::create(exec, this, 0, String(), globalFuncThrowTypeError);

Why is this change from empty string to null string OK?

[Benjamin Poulain]

(In reply to comment #4)
> (From update of attachment 161817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161817&action=review
> 
> This patch looks good, but I'm wondering about these null -> empty string changes.

Every case is described in the ChangeLog.

They are in the comments per function, not at the beginning.

[Geoffrey Garen]

Comment on attachment 161817 [details]
Patch

Reading is fundamental! :)

r=me

[Benjamin Poulain]

Created attachment 161847 [details]
Patch

[Benjamin Poulain]

Created attachment 161849 [details]
Patch

[Benjamin Poulain]

GCC 4.2 has a bug with CFSTR and strict-aliasing. I don't really get why it believes there is aliasing there...

Anyway, I added a workaround for EWS, just using the old code. I think every other bot use Clang.

[Benjamin Poulain]

Created attachment 162079 [details]
Patch

[Benjamin Poulain]

Committed r127505: <http://trac.webkit.org/changeset/127505>