Don't lookup the cache for the result of Function::New

Don't lookup the cache for the result of Function::New

Since isFunctionCached condition is wrong, we lookup the cache even if
doNotCache is true. As a result, Function::New always returns null
except for the first time.

BUG=272579
R=dcarney@chromium.org, mstarzinger@chromium.org, yhirano@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=16737

[yusukesuzuki, 2013-09-13 05:52:00]

https://chromiumcodereview.appspot.com/23513048/diff/1/src/apinatives.js
File src/apinatives.js (right):

https://chromiumcodereview.appspot.com/23513048/diff/1/src/apinatives.js#newc...
src/apinatives.js:76: cache[serialNumber] = null;
Because of this, `(serialNumber in cache) && (cache[serialNumber] !=
kUninitialized)` becomes true even if doNotCache is true. So isFunctionCached
should check `!doNotCache` at the same time.

https://chromiumcodereview.appspot.com/23513048/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://chromiumcodereview.appspot.com/23513048/diff/1/test/cctest/test-api.c...
test/cctest/test-api.cc:20587: CHECK(elm->IsNull());
This check expects `cache[serialNumber] = null;` is executed even if doNotCache
is true.
So I left `cache[serialNumber] = null;` code.

[yhirano, 2013-09-13 05:54:56]

Thanks! lgtm.

https://codereview.chromium.org/23513048/diff/1/src/apinatives.js
File src/apinatives.js (right):

https://codereview.chromium.org/23513048/diff/1/src/apinatives.js#newcode73
src/apinatives.js:73: !doNotCache && (serialNumber in cache) &&
(cache[serialNumber] != kUninitialized);
Let's wrap this line.

[yusukesuzuki, 2013-09-13 06:03:06]

https://chromiumcodereview.appspot.com/23513048/diff/1/src/apinatives.js
File src/apinatives.js (right):

https://chromiumcodereview.appspot.com/23513048/diff/1/src/apinatives.js#newc...
src/apinatives.js:73: !doNotCache && (serialNumber in cache) &&
(cache[serialNumber] != kUninitialized);
On 2013/09/13 05:54:56, yhirano wrote:
> Let's wrap this line.

Done.

[dcarney, 2013-09-13 06:42:17]

lgtm

thanks for fixing this!  I should have added more tests when i created the api.

[Michael Starzinger, 2013-09-13 08:59:08]

https://codereview.chromium.org/23513048/diff/10001/src/apinatives.js
File src/apinatives.js (right):

https://codereview.chromium.org/23513048/diff/10001/src/apinatives.js#newcode77
src/apinatives.js:77: cache[serialNumber] = null;
Couldn't we just remove this initialization to "null" here? Would that solve the
problem? Frankly I don't understand why the cache field needs to be initialized
to "null" here.

[yusukesuzuki, 2013-09-13 10:46:19]

On 2013/09/13 08:59:08, Michael Starzinger wrote:
> https://codereview.chromium.org/23513048/diff/10001/src/apinatives.js
> File src/apinatives.js (right):
> 
>
https://codereview.chromium.org/23513048/diff/10001/src/apinatives.js#newcode77
> src/apinatives.js:77: cache[serialNumber] = null;
> Couldn't we just remove this initialization to "null" here? Would that solve
the
> problem? Frankly I don't understand why the cache field needs to be
initialized
> to "null" here.

I think there's no problem. We can remove this initialization.
But my concern is that cache[0] becomes array hole. Is there any perf regression
about it?

[dcarney, 2013-09-16 07:31:58]

> I think there's no problem. We can remove this initialization.
> But my concern is that cache[0] becomes array hole. Is there any perf
regression
> about it?

There's no regression to worry about.

[dcarney, 2013-09-16 07:31:58]

> I think there's no problem. We can remove this initialization.
> But my concern is that cache[0] becomes array hole. Is there any perf
regression
> about it?

There's no regression to worry about.

[yusukesuzuki, 2013-09-16 07:51:57]

Thanks for your reply.
So I'll change my patch.

[yusukesuzuki, 2013-09-16 08:16:54]

I've updated the patch.
PTAL

[Michael Starzinger, 2013-09-16 14:14:38]

LGTM. Thanks for the fix. I'll land this for you.

[Michael Starzinger, 2013-09-16 14:50:09]

Committed patchset #4 manually as r16737 (presubmit successful).