The tokenizer calls nsCString::FindChar 800000 on the jdk index page (see the url). Each call to nsCString::FindChar results in three function calls. This is 12% of the time in the tokenizer and can probably be reduced by half just by not going through all three levels of function calls.

I think this might be something that will be fixed when Alec is finished with the string classes.

yeah, this has been simplified somewhat, but it's still ugly. I haven't really touched FindChar lately though, so if someone wants to submit a patch, I'll gladly review it.

Here we go. I've fixed nsCString::FindChar to not call through three layers of indirection. Also, that allowed me to remove code from the general FindChar and I removed a test that was always true. This caused nsStr::FindChar1 to be called only by debug code, but I don't dare putting that function under an NS_DEBUG define. I don't know how binary compatibility and such will suffer from such a move. It's not virtual so I wouldn't expect any bad things to happen, but I'm no expert on binary compatibility. Anyway, this works. jag, alecf, can you review this?

Comment on attachment 84149 [details] [diff] [review] Cleaned up version of nsCString::FindChar This is fantastic! How do you feel about cleaning up nsString::FindChar in a similar manner? :) nsStrPrivate::FindChar1 is just that - private - its not even exported from the DLL, and the headers are not exported to dist/include/string. You can remove it, or #ifdef DEBUG it - in fact I'd love to see that happen! sr=alecf on this part of the patch, but lets not close this bug until nsString is fixed, and FindChar1 is made debug-only

Comment on attachment 84149 [details] [diff] [review] Cleaned up version of nsCString::FindChar You seem to be mixing |a+b| and |a + b|, same for |=|. Could you stick to one style? r=jag

I will go ahead and take all the find functions in one go. This and bug 92709. I'm running with it now, but I want to really study all the code to see that no bugs slip in before I attach it for review. I think I will be done this weekend. I will, unless reviewers say otherwise, remove one of the FindCharInSet functions that had only one caller in the Mozilla tree, and that caller could more efficient, with less code use one of the other FindCharInSet functions. (It used nsCString::FindCharInSet(const nsCString& aSet, ...) when it should have used nsCString::FindCharInSet(const char* aSet, ...).)

Here's a patch for this bug and bug 92709. After thinking I kept the functions that's not used in the Mozilla code base but might been use elsewhere, except for the functions that were missing definitions. Yes, there was a couple of FindInCharSet functions that would have caused linking errors if ever used because they were never implemented. The FindChar implementation are ~1.5-2x as fast as the current and FindCharInSet is ~5-10x as fast. I've ran with it for two weeks and haven't seen any problems at all.

Daneil, your work is really appreciate. I write one simple test cpp to verify your patch 84149's improvement to perf. just create one simple nsAutoString, then call 1000000 times FindChar, record the time before call and after call, then get the total function times. I run it on solaris, I find that without your patch, the total time is 627286, with your patch, the total time is 611134. got 2% percent improvement. not as you said 1.5-2.x faster.

That depends on the string you're searching in. I'me measuring the time that is spent in FindChar in Mozilla. First, I guess you used an nsAutoCString because nsString.FindChar didn't exist before this patch. Second, both the new and the old implementation goes down to a memchr call where the majority of the time should be spent. The difference is that before there were 2-3 layers of function calls before coming down to the memchr which, when searching short strings, was a big overhead. When searching long strings that overhead wouldn't be significant. I have no idea how long a "long string" is. Then, Quantify could be lying to me. Anyway, I'm much more interested in the FindCharInSet performance. Can you do some measurements there too? I know it's faster since it (the same algorithm) made page load faster when implemented in the scanner, but I don't know quite how much faster. The inner loop is 15 machine instructions in a debug x86 build. In an optimized build I think it could be as low as 5-6 instructions. Remember that it's optimized towards searching for "special" chars.

*** Bug 92709 has been marked as a duplicate of this bug. ***

Daniel, what is the status of this bug? I have written one simple testcase, based on http://www.mozilla.org/newlayout/testcases/stress/wbtbltxt.html, just add some js to calculate the start time before the table, end time after the table, then end time - start time = table load time. this may be not accurate, but can be an reference. my test result is that: before I apply your patch of first one, table load time = 3631 after I apply your patch , table load time = 3356 got about 7% percent improment for this testcase. because this testcase is stress test, I think for an normal page, the improvement can not be 7%. why not check it in since it is an good patch and improve page-loading perf ?

The status is that it's waiting for review (alecf, jag... can you look at it?). For the moment I have only limited access to mail and computers so I haven't taken the time to push for it.

Btw, that performance increase seems too big. Yes, this makes FindCharInSet faster, but we don't do _that_ much FindCharInSet from the beginning. At least we shouldn't do.

7% improvement only for my testcase. for it is 100x100 table, contains 10000 text. so the improvement seems large.

Comment on attachment 85948 [details] [diff] [review] Patch for FindChar and FindCharInSet This patch seems give a big improvement. Can anyone r=? Thx.

Comment on attachment 85948 [details] [diff] [review] Patch for FindChar and FindCharInSet seems quite reasonable to me.

jag, can you still give r?

Comment on attachment 85948 [details] [diff] [review] Patch for FindChar and FindCharInSet r=jag

can we get this in quite soon?

Since Daniel's on vacation, it can be a while.

in trunk