Bug 23128 - get/put_by_val need to respecialise in the face of ByteArray
Summary: get/put_by_val need to respecialise in the face of ByteArray
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Oliver Hunt
URL: http://nerget.com/canvas/perfTest.html
Depends on:
Reported: 2009-01-05 22:32 PST by Oliver Hunt
Modified: 2009-01-13 18:12 PST (History)
2 users (show)

See Also:

Specialise get/put_by_val (6.84 KB, patch)
2009-01-11 07:13 PST, Oliver Hunt
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2009-01-05 22:32:21 PST
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%
Comment 1 Oliver Hunt 2009-01-11 07:13:24 PST
Created attachment 26605 [details]
Specialise get/put_by_val

Nice and simple
Comment 2 Anders Carlsson 2009-01-11 08:41:59 PST
Comment on attachment 26605 [details]
Specialise get/put_by_val

Comment 3 Oliver Hunt 2009-01-11 08:49:08 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/interpreter/Interpreter.cpp
	M	JavaScriptCore/interpreter/Interpreter.h
Committed r39796

Comment 4 Darin Adler 2009-01-11 09:04:08 PST
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.