Bug 93193 - Use the initialization from literal for JSC's Identifiers
Summary: Use the initialization from literal for JSC's Identifiers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-04 20:02 PDT by Benjamin Poulain
Modified: 2012-08-07 15:21 PDT (History)
1 user (show)

See Also:


Attachments
Patch (12.50 KB, patch)
2012-08-04 20:31 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (9.71 KB, patch)
2012-08-06 19:59 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-08-04 20:02:24 PDT
That should save some initialization time and some memory.
Comment 1 Benjamin Poulain 2012-08-04 20:31:31 PDT
Created attachment 156550 [details]
Patch
Comment 2 Build Bot 2012-08-04 20:56:31 PDT
Comment on attachment 156550 [details]
Patch

Attachment 156550 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13436356
Comment 3 Geoffrey Garen 2012-08-05 11:50:07 PDT
Comment on attachment 156550 [details]
Patch

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

r- for build failure. Needs an export, I think:

4>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: static class WTF::PassRefPtr<class WTF::StringImpl> __cdecl JSC::Identifier::add(class JSC::ExecState *,char const *)" (?add@Identifier@JSC@@SA?AV?$PassRefPtr@VStringImpl@WTF@@@WTF@@PAVExecState@2@PBD@Z)

Is this measurably faster on some benchmark?

> Source/JavaScriptCore/runtime/Identifier.cpp:153
> +    const LiteralIdentifierTable::iterator& iter = literalIdentifierTable.find(c);
> +    if (iter != literalIdentifierTable.end())
> +        return iter->second;
> +
> +    CharBuffer<LChar> charBuffer = { reinterpret_cast<const LChar*>(c), length };
> +    HashSet<StringImpl*>::AddResult addResult = identifierTable.add<CharBuffer<LChar>, IdentifierASCIICharBufferTranslator>(charBuffer);

I wonder if there's a way to avoid computing the string hash twice here.
Comment 4 Benjamin Poulain 2012-08-05 12:47:19 PDT
> r- for build failure. Needs an export, I think:

Yep, damn Windows :(


> Is this measurably faster on some benchmark?

The change is mostly for memory usage, I have only measured it does not regress.

If the QualifiedName change is any indication, this is also likely faster at initialization. I doubt that shows up in any benchmark, this likely only affects startup time.

I'll see if I can DTrace on ARM to get some numbers for CPU time.


> I wonder if there's a way to avoid computing the string hash twice here.

Actually, they are not the same hash. The literalIdentifierTable hashes the pointer to the string, the identifierTable hashes the string itself.
Comment 5 Benjamin Poulain 2012-08-06 19:59:28 PDT
Created attachment 156844 [details]
Patch
Comment 6 Benjamin Poulain 2012-08-06 20:05:29 PDT
> Is this measurably faster on some benchmark?

It is actually also measurable in time. On http://pmav.eu/stuff/javascript-webworkers/ , if you put enough workers, you get a small improvement (~0.9% on the smallest workset).

I gave up on the version that hardcodes the string length. The problem is that was growing the binary since the size is hardcoded in the text segment. Growing the binary size to improve launch time is about as bad as it can get, so I settled for this simpler version.
Comment 7 Geoffrey Garen 2012-08-06 21:52:00 PDT
Comment on attachment 156844 [details]
Patch

r=me
Comment 8 Benjamin Poulain 2012-08-07 14:46:12 PDT
Comment on attachment 156844 [details]
Patch

Clearing flags on attachment: 156844

Committed r124922: <http://trac.webkit.org/changeset/124922>
Comment 9 Benjamin Poulain 2012-08-07 14:46:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2012-08-07 15:21:55 PDT
Comment on attachment 156844 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonIdentifiers.cpp:30
> +    : nullIdentifier()

Maybe we can remove this initializer too.