Bug 239427

Created attachment 457763 [details]
Patch

Comment on attachment 457763 [details]
Patch

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

From bindings we could have AtomString&& instead of const AtomString& for cases where we are taking ownership and want to save some reference count churn. Not sure how important this is to performance, but it is an optimization possibility.

> Source/WebCore/svg/SVGLengthValue.cpp:276
> +    StringBuilder builder;
> +    builder.append(m_valueInSpecifiedUnits, lengthTypeToString(m_lengthType));
> +    return builder.toAtomString();

We should create makeAtomString for cases like this one. It’s a bit wasteful to use StringBuilder when we know everything up front, and it’s not too hard to write a makeAtomString.

(In reply to Darin Adler from comment #2)
> Comment on attachment 457763 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457763&action=review
> 
> From bindings we could have AtomString&& instead of const AtomString& for
> cases where we are taking ownership and want to save some reference count
> churn. Not sure how important this is to performance, but it is an
> optimization possibility.

Yes, Sam mentioned the same thing on Slack. I'll likely look into this in the near future.

> > Source/WebCore/svg/SVGLengthValue.cpp:276
> > +    StringBuilder builder;
> > +    builder.append(m_valueInSpecifiedUnits, lengthTypeToString(m_lengthType));
> > +    return builder.toAtomString();
> 
> We should create makeAtomString for cases like this one. It’s a bit wasteful
> to use StringBuilder when we know everything up front, and it’s not too hard
> to write a makeAtomString.

Let me see how hard it can to write.

(In reply to Chris Dumez from comment #3)
> (In reply to Darin Adler from comment #2)
> > Comment on attachment 457763 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=457763&action=review
> > 
> > From bindings we could have AtomString&& instead of const AtomString& for
> > cases where we are taking ownership and want to save some reference count
> > churn. Not sure how important this is to performance, but it is an
> > optimization possibility.
> 
> Yes, Sam mentioned the same thing on Slack. I'll likely look into this in
> the near future.
> 
> > > Source/WebCore/svg/SVGLengthValue.cpp:276
> > > +    StringBuilder builder;
> > > +    builder.append(m_valueInSpecifiedUnits, lengthTypeToString(m_lengthType));
> > > +    return builder.toAtomString();
> > 
> > We should create makeAtomString for cases like this one. It’s a bit wasteful
> > to use StringBuilder when we know everything up front, and it’s not too hard
> > to write a makeAtomString.
> 
> Let me see how hard it can to write.

I'll follow-up for this.

(In reply to Chris Dumez from comment #4)
> I'll follow-up for this.

Basically makeAtomString is really only an optimization for short strings.

Once we know the string is short enough we can use an on-stack buffer, build the entire string on the stack, look up the existing StringImpl and allocate a new one only if it’s not already in the AtomString table.

For a longer string, since we have to allocate on the heap anyway, I don’t think we can do much better than AtomString(makeString(xxx)), allocating a buffer (a StringImpl, in fact) and then deallocating it if it turns out to be identical to one in the table.

Committed r292960 (249725@main): <https://commits.webkit.org/249725@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457763 [details].

<rdar://problem/91903893>