[Webkit-unassigned] [Bug 38661] Persist V8's ScriptData to cache

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38661


Tony Gentilcore <tonyg at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ager at chromium.org




--- Comment #6 from Tony Gentilcore <tonyg at chromium.org>  2010-05-28 11:18:11 PST ---
(In reply to comment #5)
> (From update of attachment 57269 [details])
> This looks good.  I'm sad about the magic number, as we've discussed before.

As far as I can tell it is either (1) magic number or (2) enum that lives in WebCore with value names that represent things outside of WebCore. Initially you didn't like such an enum, but if you think it beats the magic number approach, I'm happy to switch back to that.

  Also, you're comments aren't they way we usually do things in WebKit-land. 

Thanks. I've gone back to some more spartan comments that only explain "why". Let me know if they need more tweaking.

 Maybe worth tweaking slightly before landing?
> 
> WebCore/ChangeLog:12
>  +          Chromium's morejs benchmark shows a 3-4% improvement on fast hardware.
> Awesome.

It appears even faster now without that silly double UTF8 encoding. I'm anxious to see what it looks like on the bots.
> 
> WebCore/bindings/v8/V8Proxy.cpp:345
>  +      static const unsigned dataTypeID = 0xECC13BD7;
> :(
> 
> WebCore/bindings/v8/V8Proxy.cpp:353
>  +      // If there is cached data, use it.
> We usually don't comment about *what* the code is doing, but more *why* it's doing that.
> 
> WebCore/bindings/v8/V8Proxy.cpp:359
>  +      v8::ScriptData* scriptData = v8::ScriptData::PreCompile(sourceCode.source().utf8().data(), sourceCode.source().utf8().length());
> Is the conversion to utf8 expensive?  Maybe we should do that once instead of twice?

Good catch. That was dumb.

Mads, it really seems like it would be an improvement here if I could pass the v8 external string that is already available in evaluate() instead of having to convert to utf8 here. Initially you didn't want me to modify PreCompile() to take a handle to a v8 string. Another approach would be to add an overloaded PreCompile() that takes a v8 string. What do you think?

I wouldn't have to block this patch on that, but could just switch to it after it lands.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.