Bug 99634 – [v8] IntegerCache should be removed in favor of a faster v8 API

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Eric Seidel (no email)]

[v8] IntegerCache is redundant with V8's SMI support and should be removed

It looks like we'd just need to create a version of Integer::New() which is inlined, and can take an Isolate pointer.  Then we would have exactly the same branch cost as our current IntegerCache implementation, without the extra code. :)  We'd also then be faster on all integers up to the SMI limit, instead of const int numberOfCachedSmallIntegers = 64;

[Adam Barth]

We need to add a new V8 API for creating integers quickly.

[Adam Barth]

The problem is more that v8::Integer::New needs to grab the isolate out of thread-local storage.  In any case, we should fix this by improving the V8 API.

[Adam Barth]

V8 change is in <https://codereview.chromium.org/11212004/>.  I haven't benchmarked the new API to know whether it is fast enough for our purposes.

@haraken, would you be willing to run the same benchmark you ran when you introduced the IntegerCache?

[Kentaro Hara]

(In reply to comment #2)
> The problem is more that v8::Integer::New needs to grab the isolate out of thread-local storage.  In any case, we should fix this by improving the V8 

When I was investigating the performance, another problem was the overhead to create a local handle for a cached persistent handle of SMIs. Given that V8 APIs needs to be general, V8 APIs cannot avoid creating local handles every time. On the other hand, by caching the persistent handles in the V8 binding side, we can avoid creating local handles and just return the cached persistent handles (because V8 binding knows the lifetime of Workers and their persistent handles).

(In reply to comment #3)
> V8 change is in <https://codereview.chromium.org/11212004/>.  I haven't benchmarked the new API to know whether it is fast enough for our purposes.
> 
> @haraken, would you be willing to run the same benchmark you ran when you introduced the IntegerCache?

The benchmark is Bindings/scroll-top.html

[Adam Barth]

> The benchmark is Bindings/scroll-top.html

Thanks.  I'll try that now.

[Adam Barth]

Created attachment 169316 [details]
Patch for benchmarking

[Adam Barth]

Created attachment 169317 [details]
PerformanceTest results (first three are with patch; last three are without patch)

Looks like about a 5% slow down on this benchmark.  It's a bit hard to tell because this machine is noisy for benchmarks.  I might try benchmarking it again on a more stable machine.

[Adam Barth]

For fun, I ran the benchmark with the old v8::Integer::New API, and the new API is definitely faster, just not as fast as having the integer cache.

[Kentaro Hara]

(In reply to comment #8)
> For fun, I ran the benchmark with the old v8::Integer::New API, and the new API is definitely faster, just not as fast as having the integer cache.

I would guess that the new API is still consuming time on creating a local handle by Utils::IntegerToLocal(result), which is not needed in the IntegerCache.

[Adam Barth]

> I would guess that the new API is still consuming time on creating a local handle by Utils::IntegerToLocal(result), which is not needed in the IntegerCache.

What takes the time here?

#define MAKE_TO_LOCAL(Name, From, To)                                       \
  Local<v8::To> Utils::Name(v8::internal::Handle<v8::internal::From> obj) { \
    ASSERT(obj.is_null() || !obj->IsTheHole());                             \
    return Local<To>(reinterpret_cast<To*>(obj.location()));                \
  }

I don't see a call to Local::New there.

[Kentaro Hara]

(In reply to comment #10)
> What takes the time here?
> 
> #define MAKE_TO_LOCAL(Name, From, To)                                       \
>   Local<v8::To> Utils::Name(v8::internal::Handle<v8::internal::From> obj) { \
>     ASSERT(obj.is_null() || !obj->IsTheHole());                             \
>     return Local<To>(reinterpret_cast<To*>(obj.location()));                \
>   }
> 
> I don't see a call to Local::New there.

hmm, that's true...

[Adam Barth]

I think I just need to benchmark this stuff on a more performance-stable machine.

[Adam Barth]

I benchmarked this on a more performance-stable machine, and the faster API isn't fast enough.

[Adam Barth]

Bug 100016 is a follow up to this bug.