Bug 45749 - Extend YARR JIT with beginning character look-up optimization
Summary: Extend YARR JIT with beginning character look-up optimization
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 61306
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-14 07:35 PDT by Peter Varga
Modified: 2011-05-24 13:02 PDT (History)
13 users (show)

See Also:


Attachments
proposed patch (4.21 KB, patch)
2010-09-14 07:39 PDT, Peter Varga
no flags Details | Formatted Diff | Diff
proposed patch v2 (4.30 KB, patch)
2010-09-20 04:12 PDT, Peter Varga
no flags Details | Formatted Diff | Diff
proposed patch v3 (4.09 KB, patch)
2010-10-21 08:20 PDT, Peter Varga
eric: review-
Details | Formatted Diff | Diff
proposed patch v3 (4.09 KB, patch)
2010-12-06 02:35 PST, Peter Varga
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Varga 2010-09-14 07:35:29 PDT
Search the start index of the first potential match before the pattern
matching process. Use the vector of possible beginning characters which are collected
by YARR's parser.
Comment 1 Peter Varga 2010-09-14 07:39:47 PDT
Created attachment 67548 [details]
proposed patch

Performance results:

                                      ref               mod
regexp-dna:   1.060x as fast   21.1ms +/- 1.1%   19.9ms +/- 1.1%
v8-regexp:    1.011x as slow  350.9ms +/- 0.2%  354.7ms +/- 0.1%
Comment 2 Early Warning System Bot 2010-09-14 07:54:49 PDT
Attachment 67548 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3998008
Comment 3 WebKit Review Bot 2010-09-14 08:00:33 PDT
Attachment 67548 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4019006
Comment 4 Csaba Osztrogonác 2010-09-14 08:06:00 PDT
(In reply to comment #2)
> Attachment 67548 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/3998008

It will build on Qt after https://bugs.webkit.org/show_bug.cgi?id=45748 fixed.
Comment 5 WebKit Review Bot 2010-09-14 08:48:34 PDT
Attachment 67548 [details] did not build on win:
Build output: http://queues.webkit.org/results/4017011
Comment 6 Geoffrey Garen 2010-09-14 13:53:00 PDT
Do you know why v8-regexp got slower?
Comment 7 Peter Varga 2010-09-15 07:19:01 PDT
(In reply to comment #6)
> Do you know why v8-regexp got slower?

I made some other measurements:
- I increased the number of iterations in the v8-regexp benchmark. The ratio of the difference between the two results 
didn't increase.
- I just measured with the parser's patch (https://bugs.webkit.org/show_bug.cgi?id=45748). I noticed that the slow-down still remains.
- On another machine the v8 benchmark's results were different. v8-regexp was faster than the reference but 
other tests' (which don't contain regular expressions) performance results were different. I think it is caused by deviation.

It looks like the slow-down with v8-regexp is caused by the beginning character collecting in the parser.
I'm working on this problem.
Comment 8 Geoffrey Garen 2010-09-15 10:32:52 PDT
Any idea why this would slow down the JIT but not the interpreter? Your interpreter results show a solid improvement on v8-regexp. It's surprising that the JIT wouldn't improve as well.
Comment 9 Peter Varga 2010-09-15 11:34:28 PDT
(In reply to comment #8)
> Any idea why this would slow down the JIT but not the interpreter? Your interpreter results show a solid improvement on v8-regexp. It's surprising that the JIT wouldn't improve as well.

I think this optimization haven't enough possibilities to make faster the pattern matching in case of v8-regexp test. Therefore the overhead of the character collecting is greater than the profit of the optimization in case of JIT.

However this optimization is more efficient in case of interpreter and the overhead of character collecting is same in both case, therefore the overhead should be negligible when the interpreter does the pattern matching.
Comment 10 Gavin Barraclough 2010-09-17 18:12:19 PDT
Hi Peter,

I think Michael is going to take a look over your patch, since he's been working in this area at the minute.  He has a patch on bugs.webkit.org/show_bug.cgi?id=45787 that I imagine could conflict with this, which is at commit-queue+ at the minute.  You may need to update your patches once this has landed to resolve any merge issues.

cheers,
G.
Comment 11 Peter Varga 2010-09-20 04:12:42 PDT
Created attachment 68070 [details]
proposed patch v2

updated patch for top of trunk.
Comment 12 Early Warning System Bot 2010-09-20 04:19:41 PDT
Attachment 68070 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4035090
Comment 13 WebKit Review Bot 2010-09-20 04:52:45 PDT
Attachment 68070 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4084022
Comment 14 WebKit Review Bot 2010-09-20 05:08:11 PDT
Attachment 68070 [details] did not build on win:
Build output: http://queues.webkit.org/results/4031067
Comment 15 Peter Varga 2010-09-20 09:20:10 PDT
The new performance results without the collecting of character classes:

                                       ref                mod
regexp-dna:  1.055x as fast     21.1ms +/- 1.1%    20.0ms +/- 0.0%
v8-regexp:   ??                316.6ms +/- 0.6%   317.6ms +/- 0.2%
Comment 16 Peter Varga 2010-10-21 08:20:56 PDT
Created attachment 71439 [details]
proposed patch v3

update patch.
Comment 17 Early Warning System Bot 2010-11-16 13:08:29 PST
Attachment 71439 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6117003
Comment 18 Build Bot 2010-11-16 15:18:22 PST
Attachment 71439 [details] did not build on win:
Build output: http://queues.webkit.org/results/5992105
Comment 19 Eric Seidel (no email) 2010-12-03 13:30:15 PST
Comment on attachment 71439 [details]
proposed patch v3

r- for qt and win failures.
Comment 20 Peter Varga 2010-12-06 02:35:41 PST
Created attachment 75659 [details]
proposed patch v3

I upload the previous patch again. It has failed on the last ews check because a depedency wasn't landed but now it seems ok.
Comment 21 Eric Seidel (no email) 2011-05-23 11:00:10 PDT
Oliver, Gavin:  This patch has been up for review for 6 months.  it's tiny.  Please r- it or r+ it.  I will r- this next week if it still remains undecided.
Comment 22 Gavin Barraclough 2011-05-23 14:22:49 PDT
Comment on attachment 75659 [details]
proposed patch v3

Clearing review flag pending 61306
Comment 23 Gavin Barraclough 2011-05-24 13:02:02 PDT
Marking this won't fix based on resolution to r61306.