Bug 58687 – [V8] KeepAlive is called everytime JS is executed but is not always necessary

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Greg Simon]

In the v8 bindings (V8Proxy) the Frame is kept alive every time JS is executed in case the JS code itself causes the Frame to be deleted. The Frame::keepAlive is not a free call; it schedules an async timer to fire at a later date to balance the ref() made initially in Frame::keepAlive. On loading gmail.google.com for example, > 100 async timers are scheduled.

This bug suggests an optimization to only call Frame::keepAlive when it is necessary-- when the protector ref in the proxy is the last reference held to the Frame object, which appears to be the exception not the rule.

[Greg Simon]

Created attachment 89842 [details]
Patch to only call keepAlive when necessary, not every time we call JS.

Patch to only call keepAlive when necessary in v8 bindings, not every time we call JS.

[Greg Simon]

Created attachment 90078 [details]
Patch to only call Frame::keepAlive when necessary, not every time we call into JS.

[Adam Barth]

Comment on attachment 90078 [details]
Patch to only call Frame::keepAlive when necessary, not every time we call into JS.

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

> Source/WebCore/ChangeLog:5
> +        Fix comment

What does "fix comment" mean?

> Source/WebCore/ChangeLog:11
> +        No new tests since it is an optimization of standard behavior.

Is there some performance test that shows improvement after this patch?  If so, which one and how much?

[Greg Simon]

I have not been able to get sunspider or dromaeo to react beyond a noise level with this change. 

That being said, the change eliminates ~571 calls to Timer::startOneShot in a single sunspider run. The reason I'm not seeing results is because calls to setNextFireTime entries are coalesced when they are very close together.


So... I an argue either side on this one. I like that it is eliminating scheduled timer callbacks (overhead in general), but in the existing benchmarks I'm not able to prove it makes a huge difference.

[Eric Seidel (no email)]

Is Greg a committer now?  What's the path foward here?

[Eric Seidel (no email)]

Ping?

[Adam Barth]

Comment on attachment 90078 [details]
Patch to only call Frame::keepAlive when necessary, not every time we call into JS.

Clearing R+.  I think this probably just needs a fixed up ChangeLog at this point.

[Greg Simon]

This change is in trunk via changeset 93913