brendan says we should do this.

In particular, strings have free bits; we should be able to ask a string whether it's in intStringTable, and maybe even where, if that helps with this bug.

We don't need bits if isIntString is fast enough. /be

Ah, indeed. I hadn't known that existed!

Well, I tried this... and it doesn't help the testcase in bug 551039 at all, because the literal string "1" is not an isIntString, in fact.

I remember trying this before but I have to take a look again why we didn't add the fast path for this function.

We have an unitStringTable and an IntStringTable. There shouldn't be 2 representations of the same int and so we create it with: if (u < 10) { /* To avoid two ATOMIZED JSString copies of 0-9. */ return &JSString::unitStringTable['0' + u]; } return &JSString::intStringTable[u]; So the "1" is not within the intStringTable.

(In reply to comment #7) > We have an unitStringTable and an IntStringTable. There shouldn't be 2 > representations of the same int and so we create it with: > > if (u < 10) { > /* To avoid two ATOMIZED JSString copies of 0-9. */ > return &JSString::unitStringTable['0' + u]; > } > return &JSString::intStringTable[u]; > > So the "1" is not within the intStringTable. Ah, thanks for reminding me. This is why js_StringToInt32, the JIT-called builtin, does: if (str->length() == 1) { jschar c = str->chars()[0]; if ('0' <= c && c <= '9') return c - '0'; return 0; } /be

Gregor, does the code for "1" go through the codepath you describe in comment 7? Or does that only get hit for runtime int-to-string conversions?

(In reply to comment #9) > Gregor, does the code for "1" go through the codepath you describe in comment > 7? Or does that only get hit for runtime int-to-string conversions? If you type "1" in the shell it goes through JSString::unitString(jschar c) and not JSString::intString(jsint i). The codepath I copied in comment 7 is from intString. I hope that was your question.

Ah, sure. That makes sense. I'd actually been testing with "11" (literal string) and still not seeing any speedup from the patch I attached here...

So "11" is an intString, but we don't optimize all paths that do string to int or string to number based on this fact. Right? /be

Well we went the optimize SS path. js_StringToNumber is called 6 times in SS. js_StringToInt32 is called 24566 times but every time the string is an unitString and we optimize it. What is the testcase that shows a performance hit?

There is no obvious testcase, actually. This bug was filed as part of the investigation of bug 551039 on Brendan's suggestion. If this code would never be hit for things that are actually intStrings, then there's no point adding a case for them. Brendan, literal "11" doesn't seem to be an intString as far as I can tell.

(In reply to comment #14) > Brendan, literal "11" doesn't seem to be an intString as far as I can tell. js> "11" Breakpoint 1, js_AtomizeString (cx=0x810000, str=0xbfffed10, flags=8) at ../jsatom.cpp:678 678 JS_ASSERT(!(flags & ~(ATOM_PINNED|ATOM_INTERNED|ATOM_TMPSTR|ATOM_NOCOPY))); (gdb) n 679 JS_ASSERT_IF(flags & ATOM_NOCOPY, flags & ATOM_TMPSTR); (gdb) 681 if (str->isAtomized()) (gdb) 684 size_t length = str->length(); (gdb) 685 if (length == 1) { (gdb) p length $1 = 2 (gdb) n 698 if (2 <= length && length <= 3) { (gdb) 699 const jschar *chars = str->chars(); (gdb) 701 if ('1' <= chars[0] && chars[0] <= '9' && (gdb) 704 jsint i = (chars[0] - '0') * 10 + chars[1] - '0'; (gdb) 706 if (length == 3) (gdb) 708 if (jsuint(i) < INT_STRING_LIMIT) (gdb) 709 return (JSAtom *) STRING_TO_JSVAL(JSString::intString(i)); (gdb) 788 } /be

This works fine. I have no idea why it didn't work before...

This of course still leaves the question of whether we want this change. This fast-path is about 10x faster than current slow-path, but only 4x faster if we get rid of LOCK_DTOA, I think.

static inline int32 intStringToInt(void* ptr) { The naming is kind of dangerous since "0"-"9" are not intStrings. We should add a big warning. I remember a small SS regression so I removed the optimization code last year.

> I remember a small SS regression so I removed the optimization code last year. Which code?

When we did the patch I experimented with the fast-path code. I had the fast-path code in both StringToNumber and StringToInt32 but the version with only unitString optimization in StringToInt32 was the fastest for SS on my very old MacBook Pro. But there was no difference on a MacPro. So we decided to add other fast path on demand. Maybe its time for it now :) I like this patch.

The remaining question is how big the overhead is if we only have to convert "double strings". I got some numbers for attached micro-benchmark on my Mac Pro 2.66 Dual Core. Correct me if I am measuring something else here... Tip: Time is about 670 with this patch: Time is about 687 I guess the slowdown is manageable and we should take the fast path. Any other opinions?

Over here on a macbook pro the numbers are: Tip: about 1690 With this patch: about 1750 which is the same 3% difference or so that you're seeing. I really wish we had some data on how often the fast path will be hit vs bypassed in practice....

Comment on attachment 431466 [details] [diff] [review] Updated patch >+ if (JSString::isIntString(str)) >+ return NumberTraits<T>::toSelfType(JSString::intStringToInt(str)); Can you Shark things to show this if test resulting in false, so the cost of the test and the branch around the early-returning then, is indeed the source of the regression? >+ static inline int32 intStringToInt(void* ptr) { >+ JS_ASSERT(isIntString(ptr)); >+ jsuword delta = reinterpret_cast<jsuword>(ptr) - >+ reinterpret_cast<jsuword>(intStringTable); >+ return delta / sizeof(JSString); Much prefer to use the valid JSString pointer type and let the compiler do the division. /be

I had assumed there was a reason the existing isIntString didn't take JSString*. If there isn't, so much the better.

I collected numbers for some internet pages (GMail, GDocs, CNN, Facebook....) StringToNumberType calls: 9422 str->length == 1 : 1207 ... 13% isIntString : 504 ... 5% other: : 7711 ... 82%

Sounds like outside of benchmarks this isn't very useful (especially since the speedup from the fast-path will be less once LOCK_DTOA dies).

Comment on attachment 431466 [details] [diff] [review] Updated patch >@@ -447,16 +447,19 @@ StringToNumberType(JSContext *cx, JSStri > { > if (str->length() == 1) { > jschar c = str->chars()[0]; > if ('0' <= c && c <= '9') > return NumberTraits<T>::toSelfType(int32(c - '0')); > return NumberTraits<T>::NaN(); > } > >+ if (JSString::isIntString(str)) >+ return NumberTraits<T>::toSelfType(JSString::intStringToInt(str)); >+ We should check if the str->length() == 1 should be eliminated or at least replaced with a unit string check. >+ static inline int32 intStringToInt(void* ptr) { The function should take JSString * pointer and use straight ptr - intStringTable to get the index.