HTMLTokenizer: Fold isASCIIUpper() / isASCIILower() cases

HTMLTokenizer: Fold isASCIIUpper() / isASCIILower() cases

I noticed that NextToken() had a bunch of code doing:

  if (IsASCIIUpper(c)) {
    doStuff(ToLowerCase(c));
  } else if (IsASCIILower(c)) {
    doStuff(c);
  }

Since lowercasing is done by just setting the 0x20 bit, we can fold it into the
range check and always lowercase:

  if (IsASCIIAlpha(c)) {
    doStuff(ToLowerCase(c));
  }

This actually brings the code closer to the tokenization spec which for these
states does not separate between upper- and lower-case inputs.

I tried to measure the performance of this by modifying
PerformanceTests/resources/runner.js to run 1000 iterations and running the
html-parser.html test:

$ tools/perf/run_benchmark run blink_perf.parser \
  --story-filter=html-parser.html \
  --browser-executable=/work/chromium/src/out/release/content_shell

However I could see no change in performance.

Without my change:

avg 379.1670899999993 ms
median 381.46249999999054 ms
stdev 45.56871307699382 ms
min 253.66499999997905 ms
max 1199.0950000000007 ms

With my change:

avg 376.9821549999998 ms
median 381.8125 ms
stdev 42.23547601324007 ms
min 254.27000000000407 ms
max 1127.37 ms

This does remove ~100 lines of code and shave ~1 KB off the object file size
which is always something. And it should be faster, really.

BUG=none

Review-Url: https://codereview.chromium.org/2827653003
Cr-Commit-Position: refs/heads/master@{#465690}
Committed: https://chromium.googlesource.com/chromium/src/+/d2e7b9cf0ef86f473dfce65d0317ead63e7984a8

[hans, 2017-04-18 22:37:29]

hans@chromium.org changed reviewers:
+ csharrison@chromium.org, thakis@chromium.org

[hans, 2017-04-18 22:37:30]

Please take a look.

And let me know if you have good ideas for a convenient way to micro-benchmark
this function.

[Charlie Harrison, 2017-04-19 00:03:38]

Code wise this LGTM, and is a great cleanup.

Have you tried analyzing this with a profiler? I suspect it might be tricky due
to the aggressive inlining. I would also recommend just measuring the tokenizer
steps in about:tracing.

+kouhei any ideas for how to microbenchmark the tokenizer?

https://codereview.chromium.org/2827653003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLTokenizer.cpp (right):

https://codereview.chromium.org/2827653003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLTokenizer.cpp:241: if (cc == '!')
{
Thanks for fixing up the style issues here. This code has been unloved for a
while.

[kouhei (in TOK), 2017-04-19 00:52:27]

lgtm

Thanks for the clean up!

> Have you tried analyzing this with a profiler? I suspect it might be tricky
due
> to the aggressive inlining. I would also recommend just measuring the
tokenizer
> steps in about:tracing.
+1. I think this is the easiest way. Add TRACE_EVENT macros as needed.

[hans, 2017-04-19 16:44:19]

I can see the BackgroundHTMLParser::pumpTokenizer event in about:tracing, but
it's not clear what to do with them?

Since we don't have any benchmark for the tokenizer, I don't think there's more
to do here. I'll take it to the cq.

[hans, 2017-04-19 16:44:24]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2017-04-19 16:45:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 16:49:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 16:49:46]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Charlie Harrison, 2017-04-19 16:51:12]

You can use the tracing UI to measure the duration of the tokenize step. If you
double click on a trace, it will also show you the duration of *all* the events
summed up.

This can give a less noisy signal than the total parse time / load time.

In any case, feel free to CQ.

[Charlie Harrison, 2017-04-19 16:53:52]

Just curious, how did you notice this inefficiency? Just by scanning the code?

[hans, 2017-04-19 17:11:14]

On 2017/04/19 16:53:52, Charlie Harrison wrote:
> Just curious, how did you notice this inefficiency? Just by scanning the code?

I was profiling Speedometer, and blink::HTMLTokenizer::NextToken showed up as
the 5th hottest function at 1.05% (after the V8 generated code, the profile is
pretty flat), so I took a look at the code and saw that handling upper- and
lowercase characters separately but doing essentially the same thing for them
seemed inefficient.

[hans, 2017-04-19 17:11:18]

The CQ bit was checked by hans@chromium.org

[hans, 2017-04-19 17:11:19]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org,
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2827653003/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-19 17:11:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 19:05:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492621878594490,
"parent_rev": "45d7b0fd5122143bca4dfc84370e11d7594cd411", "commit_rev":
"d2e7b9cf0ef86f473dfce65d0317ead63e7984a8"}

[commit-bot: I haz the power, 2017-04-19 19:06:01]

Description was changed from

==========
HTMLTokenizer: Fold isASCIIUpper() / isASCIILower() cases

I noticed that NextToken() had a bunch of code doing:

  if (IsASCIIUpper(c)) {
    doStuff(ToLowerCase(c));
  } else if (IsASCIILower(c)) {
    doStuff(c);
  }

Since lowercasing is done by just setting the 0x20 bit, we can fold it into the
range check and always lowercase:

  if (IsASCIIAlpha(c)) {
    doStuff(ToLowerCase(c));
  }

This actually brings the code closer to the tokenization spec which for these
states does not separate between upper- and lower-case inputs.

I tried to measure the performance of this by modifying
PerformanceTests/resources/runner.js to run 1000 iterations and running the
html-parser.html test:

$ tools/perf/run_benchmark run blink_perf.parser \
  --story-filter=html-parser.html \
  --browser-executable=/work/chromium/src/out/release/content_shell

However I could see no change in performance.

Without my change:

avg 379.1670899999993 ms
median 381.46249999999054 ms
stdev 45.56871307699382 ms
min 253.66499999997905 ms
max 1199.0950000000007 ms

With my change:

avg 376.9821549999998 ms
median 381.8125 ms
stdev 42.23547601324007 ms
min 254.27000000000407 ms
max 1127.37 ms

This does remove ~100 lines of code and shave ~1 KB off the object file size
which is always something. And it should be faster, really.

BUG=none
==========

to

==========
HTMLTokenizer: Fold isASCIIUpper() / isASCIILower() cases

I noticed that NextToken() had a bunch of code doing:

  if (IsASCIIUpper(c)) {
    doStuff(ToLowerCase(c));
  } else if (IsASCIILower(c)) {
    doStuff(c);
  }

Since lowercasing is done by just setting the 0x20 bit, we can fold it into the
range check and always lowercase:

  if (IsASCIIAlpha(c)) {
    doStuff(ToLowerCase(c));
  }

This actually brings the code closer to the tokenization spec which for these
states does not separate between upper- and lower-case inputs.

I tried to measure the performance of this by modifying
PerformanceTests/resources/runner.js to run 1000 iterations and running the
html-parser.html test:

$ tools/perf/run_benchmark run blink_perf.parser \
  --story-filter=html-parser.html \
  --browser-executable=/work/chromium/src/out/release/content_shell

However I could see no change in performance.

Without my change:

avg 379.1670899999993 ms
median 381.46249999999054 ms
stdev 45.56871307699382 ms
min 253.66499999997905 ms
max 1199.0950000000007 ms

With my change:

avg 376.9821549999998 ms
median 381.8125 ms
stdev 42.23547601324007 ms
min 254.27000000000407 ms
max 1127.37 ms

This does remove ~100 lines of code and shave ~1 KB off the object file size
which is always something. And it should be faster, really.

BUG=none

Review-Url: https://codereview.chromium.org/2827653003
Cr-Commit-Position: refs/heads/master@{#465690}
Committed:
https://chromium.googlesource.com/chromium/src/+/d2e7b9cf0ef86f473dfce65d0317...
==========

[commit-bot: I haz the power, 2017-04-19 19:06:02]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d2e7b9cf0ef86f473dfce65d0317...