Bug 53665 - Make XSSFilter go fast by adding a SuffixTree
Summary: Make XSSFilter go fast by adding a SuffixTree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 49845
  Show dependency tree / graph
 
Reported: 2011-02-03 00:45 PST by Adam Barth
Modified: 2011-02-04 02:40 PST (History)
4 users (show)

See Also:

Attachments
Patch (3.29 KB, patch)
2011-02-03 00:46 PST, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (2.99 KB, patch)
2011-02-03 16:11 PST, Adam Barth 		eric: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-02-03 00:45:40 PST 
Make XSSFilter go fast by adding a SuffixTree
Comment 1 Adam Barth 2011-02-03 00:46:46 PST 
Created attachment 81038 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-02-03 00:48:42 PST 
Comment on attachment 81038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81038&action=review

> Source/WebCore/html/parser/XSSFilter.cpp:146
> +    const size_t miniumLengthForSuffixTree = 512; // FIXME: Tune this parameter.

How would we tune it?

> Source/WebCore/html/parser/XSSFilter.cpp:183
> +                m_decodedHTTPBodySuffixTree = adoptPtr(new SuffixTree<ASCIICodebook>(m_decodedHTTPBody, suffixTreeDepth));

No create?
Comment 3 Adam Barth 2011-02-03 00:54:24 PST 
Comment on attachment 81038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81038&action=review

>> Source/WebCore/html/parser/XSSFilter.cpp:146
>> +    const size_t miniumLengthForSuffixTree = 512; // FIXME: Tune this parameter.
> 
> How would we tune it?

Dunno.  With some sort of benchmark.

>> Source/WebCore/html/parser/XSSFilter.cpp:183
>> +                m_decodedHTTPBodySuffixTree = adoptPtr(new SuffixTree<ASCIICodebook>(m_decodedHTTPBody, suffixTreeDepth));
> 
> No create?

It's adoptPtr, not adoptRef.  We could add a create method, but we don't usually.
Comment 4 Adam Barth 2011-02-03 16:11:21 PST 
Created attachment 81134 [details]
Patch
Comment 5 Eric Seidel (no email) 2011-02-03 16:24:26 PST 
Comment on attachment 81134 [details]
Patch

What's the perf improvement?
Comment 6 Adam Barth 2011-02-03 16:26:17 PST 
https://bugs.webkit.org/show_bug.cgi?id=36694
Comment 7 Adam Barth 2011-02-03 16:29:00 PST 
We can add these benchmarks to PerformanceTests/XSSFilter if you like.
Comment 8 Eric Seidel (no email) 2011-02-03 16:30:03 PST 
(In reply to comment #7)
> We can add these benchmarks to PerformanceTests/XSSFilter if you like.

I would very much like that.  As is, there is too much hidden-knowledge around the filter.  Adding the perf tests to a common place makes it easier for others to hack on this (and possibly make it even faster).
Comment 9 Eric Seidel (no email) 2011-02-03 16:31:02 PST 
Comment on attachment 81134 [details]
Patch

OK.  I look forward to you landing the perf tests.
Comment 10 Adam Barth 2011-02-03 16:42:13 PST 
Committed r77560: <http://trac.webkit.org/changeset/77560>
Comment 11 Adam Barth 2011-02-03 18:41:18 PST 
Perf test in https://bugs.webkit.org/show_bug.cgi?id=53741
Comment 12 WebKit Review Bot 2011-02-04 02:40:44 PST 
http://trac.webkit.org/changeset/77560 might have broken GTK Linux 32-bit Release, GTK Linux 32-bit Debug, and GTK Linux 64-bit Debug
The following tests are not passing:
fast/events/pagehide-timeout.html