Bug 165031 – Remove unneeded additional level of short strings optimization in LiteralParser

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[Darin Adler]

Remove unneeded additional level of optimization in LiteralParser

[Darin Adler]

Created attachment 295338 [details]
Patch

[Darin Adler]

Yusuke, I would like to know your thoughts about this. Am I correct that this is not needed, or did I miss something?

[Darin Adler]

Comment on attachment 295338 [details]
Patch

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

> Source/JavaScriptCore/runtime/LiteralParser.cpp:133
> +    if (length < 2 || characters[0] >= MaximumCachableCharacter)

I think length <= 1 would be easier to understand than length < 2.

> Source/JavaScriptCore/runtime/LiteralParser.h:198
> +    static constexpr unsigned MaximumCachableCharacter = 128;

Just realized this name is not accurate. The maximum "cachable character" is 127, not 128.

[Yusuke Suzuki]

(In reply to comment #2)
> Yusuke, I would like to know your thoughts about this. Am I correct that
> this is not needed, or did I miss something?

Agreed. Yeah, we already have the optimization path for empty & 1 length Identifiers in Identifier::add. So, these optimization seems duplicate.

BTW, we have similar mechanism for JS parser. You can find it in ParserArena's makeIdentifier. While they need to hold all the identifiers in m_identifiers (to keep its lifetime), caching mechanism by m_recentIdentifiers is the same.

Can you drop this thing too?

And I think we need to take some performance numbers. Once JS parser change is also done, I recommend to run Octane's CodeLoad. It's parser heavy benchmark.
And I also recommend to run kraken and its json benchmark. (But maybe, this benchmark does not contain empty string).

[Yusuke Suzuki]

BTW, String::empty is static function, so it will be converted to function calls.
If it causes slow down, returning vm.propertyNames->emptyIdentifier in Identifier is one idea.

[Darin Adler]

(In reply to comment #4)
> BTW, we have similar mechanism for JS parser. You can find it in
> ParserArena's makeIdentifier. While they need to hold all the identifiers in
> m_identifiers (to keep its lifetime), caching mechanism by
> m_recentIdentifiers is the same.
> 
> Can you drop this thing too?

I think the situation is different in ParserArena; when we get something out of ParserArena::m_shortIdentifiers, we don't need to add it to ParserArena::m_identifiers again because we already know it’s in there. I suspect that optimization is worthwhile; avoids the possibility of needing to grow m_identifiers for example. But in LiteralParser, there is no such benefit.

On the other

> And I think we need to take some performance numbers. Once JS parser change
> is also done, I recommend to run Octane's CodeLoad. It's parser heavy
> benchmark.
> And I also recommend to run kraken and its json benchmark. (But maybe, this
> benchmark does not contain empty string).

Good suggestions.

> BTW, String::empty is static function, so it will be converted to function calls.

I knew it was a static function, but I am really surprised it’s not inlined.

> If it causes slow down, returning vm.propertyNames->emptyIdentifier in Identifier is
> one idea.

Agreed.

[Darin Adler]

Looks like the single character code path in SmallStrings also has a function call. So this patch is not quite right as-is.

[Yusuke Suzuki]

(In reply to comment #6)
> (In reply to comment #4)
> > BTW, we have similar mechanism for JS parser. You can find it in
> > ParserArena's makeIdentifier. While they need to hold all the identifiers in
> > m_identifiers (to keep its lifetime), caching mechanism by
> > m_recentIdentifiers is the same.
> > 
> > Can you drop this thing too?
> 
> I think the situation is different in ParserArena; when we get something out
> of ParserArena::m_shortIdentifiers, we don't need to add it to
> ParserArena::m_identifiers again because we already know it’s in there. I
> suspect that optimization is worthwhile; avoids the possibility of needing
> to grow m_identifiers for example. But in LiteralParser, there is no such
> benefit.

Oh, right.

> 
> On the other
> 
> > And I think we need to take some performance numbers. Once JS parser change
> > is also done, I recommend to run Octane's CodeLoad. It's parser heavy
> > benchmark.
> > And I also recommend to run kraken and its json benchmark. (But maybe, this
> > benchmark does not contain empty string).
> 
> Good suggestions.
> 
> > BTW, String::empty is static function, so it will be converted to function calls.
> 
> I knew it was a static function, but I am really surprised it’s not inlined.
> 

I think this should be inlined. The way in my mind is,

(1): defining `constexpr` StringImpl constructor
(2): defining static StringImpl for empty string
(3): returning the address of (2) in StringImpl::empty()

I'll attempt to create the patch for this.
In particular, (1) is nice. Once it is done, we can easily define static strings in C++.

> > If it causes slow down, returning vm.propertyNames->emptyIdentifier in Identifier is
> > one idea.
> 
> Agreed.

[Darin Adler]

I’ve found lots of non-inlined things that I thought were inlined. There is some work needed to make SmallStrings give inline access to the single character strings. Separately, I am confused by the many different empty strings, and particularly the issues when using empty strings on multiple threads.

[Darin Adler]

I’m not going to bother with this. At least not keeping a bug open about it.