Bug 95940 – Make the String initialization on the function side of String::number()

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

Make ref-count done on the function side of String::number()

[Patrick R. Gansterer]

Created attachment 162432 [details]
Patch

[Build Bot]

Comment on attachment 162432 [details]
Patch

Attachment 162432 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13770248

[Benjamin Poulain]

Comment on attachment 162432 [details]
Patch

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

The template generalization taking StringType is not very useful.

This remind me we should have StringBuilder::append(integer). Since you are creating generalizations, could you find a template that could do:
1) String initialization like today
2) If used in StringBuilder, call StringBuilder::append(LChar*, length) on buf.

> Source/JavaScriptCore/ChangeLog:10
> +        instead of being on the caller side. Keep the conversion code in its own
> +        file so we can reuse the function for AtomicString::number() later too.

I am pretty sure we don't need AtomicString::number().
But I actually like your idea of keeping that code separated. It will keep WTFString.cpp cleaner.

> Source/WTF/ChangeLog:20
> +        (WTF):

Not useful.

> Source/WTF/ChangeLog:25
> +        (WTF):

Ditto.

> Source/WTF/wtf/text/IntegerToStringConversion.h:46
> +template<typename StringType, typename UnsignedIntegerType, bool Negative>
> +static StringType numberToStringImpl(UnsignedIntegerType number)

I am not a fan of the template parameter for negative numbers.

I understand it is shorter, but do you thing it is clearer?

I am not against it either.

> Source/WTF/wtf/text/WTFString.h:228
> +    WTF_EXPORT_STRING_API static String number(short);

The version taking "short" was not there on purpose.

I don't remember why I left unsigned short though... Might be related to the KURL works.

> Source/WebKit2/ChangeLog:10
> +        Un-Inline String::number() to make the ref-count done on the function side
> +        instead of being on the caller side. Keep the conversion code in its own
> +        file so we can reuse the function for AtomicString::number() later too.

No need to copy the description.

> Tools/ChangeLog:10
> +        Un-Inline String::number() to make the ref-count done on the function side
> +        instead of being on the caller side. Keep the conversion code in its own
> +        file so we can reuse the function for AtomicString::number() later too.

No need to copy this for Tools.

[Patrick R. Gansterer]

Comment on attachment 162432 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        file so we can reuse the function for AtomicString::number() later too.
> 
> I am pretty sure we don't need AtomicString::number().
> But I actually like your idea of keeping that code separated. It will keep WTFString.cpp cleaner.

When removing the implicit cas from String to AtomicString you get many erros caused by this. So I wanted to add it already a few months ago, but it was too hard without code duplication because of the sprintf stuff. But I can remove the sentence from the ChangeLog, so it is not an issue here.

>> Source/WTF/wtf/text/IntegerToStringConversion.h:46
>> +static StringType numberToStringImpl(UnsignedIntegerType number)
> 
> I am not a fan of the template parameter for negative numbers.
> 
> I understand it is shorter, but do you thing it is clearer?
> 
> I am not against it either.

I am not sure, but I do not like the code duplication. Even of the 8 lines of code

>> Source/WTF/wtf/text/WTFString.h:228
>> +    WTF_EXPORT_STRING_API static String number(short);
> 
> The version taking "short" was not there on purpose.
> 
> I don't remember why I left unsigned short though... Might be related to the KURL works.

At least there is no sentece about it in the ChangeLog at http://trac.webkit.org/changeset/126658.

[Patrick R. Gansterer]

(In reply to comment #3)
> (From update of attachment 162432 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162432&action=review
> 
> The template generalization taking StringType is not very useful.
> 
> This remind me we should have StringBuilder::append(integer). Since you are creating generalizations, could you find a template that could do:
> 1) String initialization like today
> 2) If used in StringBuilder, call StringBuilder::append(LChar*, length) on buf.

I tought about it already too, but didn't wanted to extend this patch even more. But I can do, if you're willing to review a bigger one too. ;-)

[Benjamin Poulain]

Still up to check the bots :)

> > I am pretty sure we don't need AtomicString::number().
> > But I actually like your idea of keeping that code separated. It will keep WTFString.cpp cleaner.
> 
> When removing the implicit cas from String to AtomicString you get many erros caused by this. So I wanted to add it already a few months ago, but it was too hard without code duplication because of the sprintf stuff. But I can remove the sentence from the ChangeLog, so it is not an issue here.

That is interesting.

Do you remember where do we get AtomicString from numbers?

> > The version taking "short" was not there on purpose.
> 
> At least there is no sentece about it in the ChangeLog at http://trac.webkit.org/changeset/126658.

I remember some of it! :)

That was as performance tweak. Notice there is no numberToStringImpl() for shorts. That is because I could not measure any difference of performance with the version taking a 32bits parameter.

The compiler on the other hand has to generate the functions so we got extra instructions without benefits.

Now, why did I left the unsigned short in WTF::String... I don't remember. I may simply have overlooked it.


> I tought about it already too, but didn't wanted to extend this patch even more. But I can do, if you're willing to review a bigger one too. ;-)

I think this is a really nice change, I'll take time to do reviews.

Supporting StringBuilder::append(integer) is not a goal of this patch either. If you can make something nice, go for it. If not, we should go with the simpler version you already made.

[Patrick R. Gansterer]

(In reply to comment #6)
> Still up to check the bots :)
> 
> > > I am pretty sure we don't need AtomicString::number().
> > > But I actually like your idea of keeping that code separated. It will keep WTFString.cpp cleaner.
> > 
> > When removing the implicit cas from String to AtomicString you get many erros caused by this. So I wanted to add it already a few months ago, but it was too hard without code duplication because of the sprintf stuff. But I can remove the sentence from the ChangeLog, so it is not an issue here.
> 
> That is interesting.
> 
> Do you remember where do we get AtomicString from numbers?

Since all DOM attributes are AtomicString, setting/getting a string from a integer causes an conversion, even most numbers cause doubleTOstring. But I'll chek again and post the corresponding lines later. Maybe AtomicString::number(dobule) is enough.

> > > The version taking "short" was not there on purpose.
> > 
> > At least there is no sentece about it in the ChangeLog at http://trac.webkit.org/changeset/126658.
> 
> I remember some of it! :)
> 
> That was as performance tweak. Notice there is no numberToStringImpl() for shorts. That is because I could not measure any difference of performance with the version taking a 32bits parameter.
> 
> The compiler on the other hand has to generate the functions so we got extra instructions without benefits.
> 
> Now, why did I left the unsigned short in WTF::String... I don't remember. I may simply have overlooked it.

I'm ok with removing short, since cpus/compilers are optimized for 32bit, but then we should remove UnsignedIntegerTrait<short> too.

> > I tought about it already too, but didn't wanted to extend this patch even more. But I can do, if you're willing to review a bigger one too. ;-)
> 
> I think this is a really nice change, I'll take time to do reviews.
> 
> Supporting StringBuilder::append(integer) is not a goal of this patch either. If you can make something nice, go for it. If not, we should go with the simpler version you already made.

Ok, I'll try to find a usefull version first.

[Benjamin Poulain]

Update the title as the problem is not really a ref-count as I said. The problem is 2 more movq, not a add.

[Patrick R. Gansterer]

Created attachment 162577 [details]
Patch

[Build Bot]

Comment on attachment 162577 [details]
Patch

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

[Patrick R. Gansterer]

Created attachment 162678 [details]
Patch

[Build Bot]

Comment on attachment 162678 [details]
Patch

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

[Patrick R. Gansterer]

Created attachment 162717 [details]
Patch

[Build Bot]

Comment on attachment 162717 [details]
Patch

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

[Patrick R. Gansterer]

Created attachment 162730 [details]
Patch

[Benjamin Poulain]

Comment on attachment 162730 [details]
Patch

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

Looks good.

> Source/WTF/wtf/text/IntegerToStringConversion.h:56
> +template<typename T, typename UnsignedIntegerType, bool Negative>
> +static typename ConversionTrait<T>::ReturnType numberToStringImpl(UnsignedIntegerType number, typename ConversionTrait<T>::AdditionalArgumentType* additionalArgument)

Can you please change this bool with an enum { PositiveNumber, NegativeNumber }

This way the call site would be :
    numberToStringImpl<T, typename UnsignedIntegerTrait<SignedIntegerType>::Type, NegativeNumber>(-number, additionalArgument);

I think one compiler (MSVC?) have issues with enum values in templates. If that's a problem, just go with the bool.

[Benjamin Poulain]

Comment on attachment 162730 [details]
Patch

Oops, did not mean to cq+.

[Benjamin Poulain]

I am very surprised by the output. Making the function inline made WebCore grow by over 4k. Reverting the change here reduces WebCore by 400bytes.

I wonder what has changed. Any idea?

[Patrick R. Gansterer]

Created attachment 162935 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 162935 [details]
Patch

Clearing flags on attachment: 162935

Committed r127991: <http://trac.webkit.org/changeset/127991>

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.