Bug 95940 - Make the String initialization on the function side of String::number()
Summary: Make the String initialization on the function side of String::number()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks: 96030
  Show dependency treegraph
 
Reported: 2012-09-06 00:17 PDT by Patrick R. Gansterer
Modified: 2012-09-09 13:50 PDT (History)
4 users (show)

See Also:


Attachments
Patch (22.01 KB, patch)
2012-09-06 00:24 PDT, Patrick R. Gansterer
benjamin: review-
Details | Formatted Diff | Diff
Patch (21.73 KB, patch)
2012-09-06 14:03 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (22.90 KB, patch)
2012-09-06 23:08 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (23.09 KB, patch)
2012-09-07 02:49 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (24.03 KB, patch)
2012-09-07 03:39 PDT, Patrick R. Gansterer
benjamin: review+
Details | Formatted Diff | Diff
Patch (24.19 KB, patch)
2012-09-07 19:49 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2012-09-06 00:17:57 PDT
Make ref-count done on the function side of String::number()
Comment 1 Patrick R. Gansterer 2012-09-06 00:24:04 PDT
Created attachment 162432 [details]
Patch
Comment 2 Build Bot 2012-09-06 00:43:39 PDT
Comment on attachment 162432 [details]
Patch

Attachment 162432 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13770248
Comment 3 Benjamin Poulain 2012-09-06 00:47:19 PDT
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.
Comment 4 Patrick R. Gansterer 2012-09-06 00:56:54 PDT
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.
Comment 5 Patrick R. Gansterer 2012-09-06 01:00:49 PDT
(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. ;-)
Comment 6 Benjamin Poulain 2012-09-06 01:10:51 PDT
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.
Comment 7 Patrick R. Gansterer 2012-09-06 01:19:16 PDT
(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.
Comment 8 Benjamin Poulain 2012-09-06 12:27:13 PDT
Update the title as the problem is not really a ref-count as I said. The problem is 2 more movq, not a add.
Comment 9 Patrick R. Gansterer 2012-09-06 14:03:13 PDT
Created attachment 162577 [details]
Patch
Comment 10 Build Bot 2012-09-06 14:42:22 PDT
Comment on attachment 162577 [details]
Patch

Attachment 162577 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13770466
Comment 11 Patrick R. Gansterer 2012-09-06 23:08:09 PDT
Created attachment 162678 [details]
Patch
Comment 12 Build Bot 2012-09-06 23:58:53 PDT
Comment on attachment 162678 [details]
Patch

Attachment 162678 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13770633
Comment 13 Patrick R. Gansterer 2012-09-07 02:49:48 PDT
Created attachment 162717 [details]
Patch
Comment 14 Build Bot 2012-09-07 03:31:36 PDT
Comment on attachment 162717 [details]
Patch

Attachment 162717 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13769742
Comment 15 Patrick R. Gansterer 2012-09-07 03:39:30 PDT
Created attachment 162730 [details]
Patch
Comment 16 Benjamin Poulain 2012-09-07 12:59:58 PDT
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.
Comment 17 Benjamin Poulain 2012-09-07 13:00:13 PDT
Comment on attachment 162730 [details]
Patch

Oops, did not mean to cq+.
Comment 18 Benjamin Poulain 2012-09-07 13:04:54 PDT
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?
Comment 19 Patrick R. Gansterer 2012-09-07 19:49:13 PDT
Created attachment 162935 [details]
Patch
Comment 20 Patrick R. Gansterer 2012-09-09 13:50:44 PDT
Comment on attachment 162935 [details]
Patch

Clearing flags on attachment: 162935

Committed r127991: <http://trac.webkit.org/changeset/127991>
Comment 21 Patrick R. Gansterer 2012-09-09 13:50:54 PDT
All reviewed patches have been landed.  Closing bug.