Bug 23128 – get/put_by_val need to respecialise in the face of ByteArray

• Status: CLOSED
• Resolution: FIXED
• Component: JavaScriptCore

[Oliver Hunt]

Some rather ad hoc testing shows that if bytearray accesses hit the bytearray check first, image data access time would be improved by ~10-15%

[Oliver Hunt]

Created attachment 26605 [details]
Specialise get/put_by_val

Nice and simple

[Anders Carlsson]

Comment on attachment 26605 [details]
Specialise get/put_by_val

r=me

[Oliver Hunt]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/interpreter/Interpreter.cpp
	M	JavaScriptCore/interpreter/Interpreter.h
Committed r39796

[Darin Adler]

Comment on attachment 26605 [details]
Specialise get/put_by_val

I know this was already reviewed, but I have a few comments.

> +    JSValuePtr result;
> +    unsigned i;
> +    
> +    bool isUInt32 = JSImmediate::getUInt32(subscript, i);

I usually like to group the declaration of an out argument like "i" with the function call it's coming out of, rather than having it separated by a blank line.

> +    if (LIKELY(isUInt32)) {
> +        if (interpreter->isJSByteArray(baseValue) && asByteArray(baseValue)->canAccessIndex(i))
> +            return JSValuePtr::encode(asByteArray(baseValue)->getIndex(i));

You need a comment explaining why this case does a return when all the other cases use "result" instead. I have no doubt there's a good reason, perhaps to save code since there can't be an exception in this case, but withut a comment it's a subtle point many people might missing.

>              double dValue = 0;
> +            ctiPatchCallByReturnAddress(STUB_RETURN_ADDRESS, reinterpret_cast<void*>(cti_op_put_by_val_byte_array));
>              if (JSImmediate::isNumber(value)) {
>                  jsByteArray->setIndex(i, JSImmediate::getTruncatedInt32(value));
>                  return;

It seems a real shame that this code defines and initialized a double that's not used in the isNumber fast case. Can you move the definition of dValue down a bit?

> +            double dValue = 0;
> +            if (JSImmediate::isNumber(value)) {
> +                jsByteArray->setIndex(i, JSImmediate::getTruncatedInt32(value));
> +                return;
> +            } else if (fastIsNumber(value, dValue)) {
> +                jsByteArray->setIndex(i, dValue);
> +                return;
> +            } else
> +                baseValue->put(callFrame, i, value);

We normally don't do else after return. And again, I think those "return" need brief comments.