Make ref-count done on the function side of String::number()
Created attachment 162432 [details] Patch
Comment on attachment 162432 [details] Patch Attachment 162432 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13770248
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 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.
(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. ;-)
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.
(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.
Update the title as the problem is not really a ref-count as I said. The problem is 2 more movq, not a add.
Created attachment 162577 [details] Patch
Comment on attachment 162577 [details] Patch Attachment 162577 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13770466
Created attachment 162678 [details] Patch
Comment on attachment 162678 [details] Patch Attachment 162678 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13770633
Created attachment 162717 [details] Patch
Comment on attachment 162717 [details] Patch Attachment 162717 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13769742
Created attachment 162730 [details] Patch
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 on attachment 162730 [details] Patch Oops, did not mean to cq+.
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?
Created attachment 162935 [details] Patch
Comment on attachment 162935 [details] Patch Clearing flags on attachment: 162935 Committed r127991: <http://trac.webkit.org/changeset/127991>
All reviewed patches have been landed. Closing bug.