Bug 45555 - Speed up deserialisation of strings
Summary: Speed up deserialisation of strings
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Oliver Hunt
Depends on:
Reported: 2010-09-10 11:56 PDT by Oliver Hunt
Modified: 2010-09-10 13:02 PDT (History)
4 users (show)

See Also:

Patch (7.77 KB, patch)
2010-09-10 12:05 PDT, Oliver Hunt
darin: 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 2010-09-10 11:56:28 PDT
Speed up deserialisation of strings
Comment 1 Oliver Hunt 2010-09-10 12:05:13 PDT
Created attachment 67220 [details]
Comment 2 Darin Adler 2010-09-10 12:13:06 PDT
Comment on attachment 67220 [details]

> +        UString m_ident;
> +        JSValue m_string;

These are confusing names. The string is named "ident" (unnecessary abbreviation), and the JSValue is named string! How about naming one thing m_string and the other m_stringAsJSValue or m_stringWrappedInJSValue or something like that.

Also, I think the function name jsString is not so great, but I don’t have a better suggestion. Maybe something with the word "wrap" in it.

> -    bool readStringData(Identifier& ident)
> +    bool readStringData(CachedString*& ident)

You should name this string, not ident.

> -    bool readStringData(Identifier& ident, bool& wasTerminator)
> +    bool readStringData(CachedString*& ident, bool& wasTerminator)

And this.

> -            Identifier ident;
> +            CachedString* ident;

And this.
Comment 3 Early Warning System Bot 2010-09-10 12:13:51 PDT
Attachment 67220 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3908359
Comment 4 Eric Seidel (no email) 2010-09-10 12:24:03 PDT
Attachment 67220 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3955350
Comment 5 Oliver Hunt 2010-09-10 12:53:11 PDT
Committed r67222: <http://trac.webkit.org/changeset/67222>
Comment 6 WebKit Review Bot 2010-09-10 13:02:49 PDT
http://trac.webkit.org/changeset/67222 might have broken Qt Linux Release minimal