Bug 39318 – Refactor HTMLTokenizer::write to make it more readable and split out the lexer bits from flow control bits

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Refactor HTMLTokenizer::write to make it more readable and split out the lexer bits from flow control bits

[Eric Seidel (no email)]

Created attachment 56412 [details]
Patch

[WebKit Review Bot]

Attachment 56412 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/HTMLTokenizer.cpp:1688:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Maciej Stachowiak]

Comment on attachment 56412 [details]
Patch

r- for lack of performance testing.

[Eric Seidel (no email)]

Please advise how you would like me to performance test.

[Eric Seidel (no email)]

Comment on attachment 56412 [details]
Patch

I used run-pageloadtest cvs-base (as provided by Maciej), edited to run the test 10 times each (instead of 3).  I discarded the first 2 runs for each (as they seemed to be noisy).

% grep FINISHED without_patch.txt | tail -8 | awk '{s += $5; print $5} END { print "avg: ", s/NR }'
2169.3
2179.2
2173.5
2157.4
2191.8
2179.0
2157.5
2137.7
avg:  2168.18

% grep FINISHED with_patch2.txt | tail -8 | awk '{s += $5; print $5} END { print "avg: ", s/NR }'
2193.4
2126.9
2226.7
2207.0
2177.4
2191.7
2186.0
2177.1
avg:  2185.77

2185.77 - 2168.18 = 17.59

17.59 / 2168.18 = 0.00811279506

Is that convincing?

[Eric Seidel (no email)]

Adding "inline" to the newly split function makes 100 plt runs claim this is a 0.5% progression.  I think it's noise, but at least the data doesn't seem to think it's a regression.

[Eric Seidel (no email)]

Created attachment 56442 [details]
Marked advance() as inline

[WebKit Review Bot]

Attachment 56442 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/HTMLTokenizer.cpp:1688:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Created attachment 56443 [details]
results from 100 plt runs w/o the patch

[Eric Seidel (no email)]

Created attachment 56444 [details]
results from 100 plt runs w/ the inline patch

[Eric Seidel (no email)]

I think I give up.  After an afternoon of fighting our broken perf-tests, I can't prove this is better or worse.  I think it's a wash, but I can't prove it.

[Eric Seidel (no email)]

Ahha!  Wrote a new benchmark. :)

The new benchmark (soon to be attached to bug 39338) correctly shows that the orginal patch and the "inline" patch are both regressions.  But using ALWAYS_INLINE on the new function shows no change.


r59687 Release Build:

Running 20 times
Ignoring warm-up run (4184)
3700
3741
3784
3735
3726
3769
3823
3775
3770
3800
3888
3811
3859
3920
3864
3950
3997
3921
3987
4027

avg 3842.35
stdev 95.62231695582365

r59687 "inline" patch applied:

Running 20 times
Ignoring warm-up run (4447)
4000
4132
3981
4047
4048
4055
4130
4021
4105
4071
4120
4161
4133
4215
4127
4204
4148
4228
4171
4256

avg 4117.65
stdev 75.01484853014102

r59687 with ALWAYS_INLINE patch applied:

Running 20 times
Ignoring warm-up run (4130)
3690
3728
3776
3759
3732
3782
3843
3787
3786
3841
3910
3826
3878
3960
3876
3932
4020
3937
4015
4001

avg 3853.95
stdev 97.88435779020057

[Eric Seidel (no email)]

Created attachment 56461 [details]
Patch

[WebKit Review Bot]

Attachment 56461 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/HTMLTokenizer.cpp:1688:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Maciej Stachowiak]

Comment on attachment 56461 [details]
Patch

r=me

[Eric Seidel (no email)]

Committed r59749: <http://trac.webkit.org/changeset/59749>