Bug 93193 – Use the initialization from literal for JSC's Identifiers

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Benjamin Poulain]

That should save some initialization time and some memory.

[Benjamin Poulain]

Created attachment 156550 [details]
Patch

[Build Bot]

Comment on attachment 156550 [details]
Patch

Attachment 156550 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13436356

[Geoffrey Garen]

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.

[Benjamin Poulain]

> 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.

[Benjamin Poulain]

Created attachment 156844 [details]
Patch

[Benjamin Poulain]

> 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.

[Geoffrey Garen]

Comment on attachment 156844 [details]
Patch

r=me

[Benjamin Poulain]

Comment on attachment 156844 [details]
Patch

Clearing flags on attachment: 156844

Committed r124922: <http://trac.webkit.org/changeset/124922>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.