Fix word count issue for Chinese and Japanese

Fix word count issue for Chinese and Japanese

Some languages don't use spaces to separate "words", like Chinese and
Japanese. In our current word counting algorithm, one paragraph of
Chinese or Japanese could be counted as one single word, and the content
classification would fail horribly.

The quick fix here is to treat Chinese and Japanese characters as words,
and adjust it by a constant factor. The factor 0.55 is from comparing
two versions of a long article in NY Times that is available in both
English and Chinese.

Read this bug for more information:
https://crbug.com/484750

** Score changes:

CJK data set:
https://x20web.corp.google.com/~wychen/domdistillerscore/cjk/cjk.html
Average F1: 0.356 -> 0.954

No changes on other data sets:
- cleaneval-golden-data
- golden_data_with_knowledge
- page-links-golden-data
- reader-mode-golden-data
- reader-images-golden-data

** Performance impact:

The average time reported by eval server for dataset
"reader-mode-golden-data" is used as the benchmark.
To reduce noise, it is rerun for 100 times.

Before: 127.380+-0.208ms
After: 128.541+-0.198ms

Difference: 1.161+-0.287ms
Percentage: 0.91+-0.25% slower than before

BUG=484750, 483710, 483713, 485177
R=cjhopman@chromium.org, kuan@chromium.org

Committed: 7a1099255504d55d2fc2348c2d208d7679aa8195

[wychen, 2015-05-14 19:08:43]

wychen@chromium.org changed reviewers:
+ cjhopman@chromium.org

[wychen, 2015-05-14 19:08:44]

PTAL

[wychen, 2015-05-14 22:54:09]

wychen@chromium.org changed reviewers:
+ kuan@chromium.org

[wychen, 2015-05-14 22:54:10]

+kuan as another native speaker of Chinese.

[kuan, 2015-05-15 01:45:07]

https://codereview.chromium.org/1131853006/diff/20001/javatests/org/chromium/...
File javatests/org/chromium/distiller/StringUtilTest.java (right):

https://codereview.chromium.org/1131853006/diff/20001/javatests/org/chromium/...
javatests/org/chromium/distiller/StringUtilTest.java:27:
assertTrue(StringUtil.countWords("ファイナルファンタジー") < 11);
this is a katakana sentence, how about also adding one for hiragana since u
mentioned that in the code? u only tested a single hiragana character @33.

what about kanji, and simplified chinese?

[cjhopman, 2015-05-15 20:16:55]

https://codereview.chromium.org/1131853006/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/StringUtil.java:45: public static native boolean
containsWordCharacter(String s) /*-{
We need to ensure that

containsWordCharacter(s) == (countWords(s) > 0)

for all strings. 

If containsWordCharacter isn't used, we could just delete it.

https://codereview.chromium.org/1131853006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/StringUtil.java:49: public static native int
countWords(String s) /*-{
Maybe we should change the name of this to reflect that it's not really counting
words anymore.

https://codereview.chromium.org/1131853006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/StringUtil.java:49: public static native int
countWords(String s) /*-{
Does this new approach make sense everywhere that we use countWords?

[wychen, 2015-05-18 18:49:20]

https://codereview.chromium.org/1131853006/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/StringUtil.java:45: public static native boolean
containsWordCharacter(String s) /*-{
On 2015/05/15 20:16:55, cjhopman wrote:
> We need to ensure that
> 
> containsWordCharacter(s) == (countWords(s) > 0)
> 
> for all strings. 
> 
> If containsWordCharacter isn't used, we could just delete it.

Deleted.

https://codereview.chromium.org/1131853006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/StringUtil.java:49: public static native int
countWords(String s) /*-{
On 2015/05/15 20:16:55, cjhopman wrote:
> Maybe we should change the name of this to reflect that it's not really
counting
> words anymore.

Well, the goal of this function is still to count words, but it is not feasible
to accurately count words for some languages. For those difficult languages,
character count is used as an approximation (adjusted by a factor). I've added
comments to this function to explain this.

https://codereview.chromium.org/1131853006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/StringUtil.java:49: public static native int
countWords(String s) /*-{
On 2015/05/15 20:16:55, cjhopman wrote:
> Does this new approach make sense everywhere that we use countWords?

One problem I see is the title finding part. It contains some magic numbers that
are inferred from English corpus, which should still work for Chinese and
Japanese after this CL. We might want to make it more robust in a separate CL
though, and also enhance eval server and dataset to validate that change. Or
validate that our current magic numbers are actually robust enough.

https://codereview.chromium.org/1131853006/diff/20001/javatests/org/chromium/...
File javatests/org/chromium/distiller/StringUtilTest.java (right):

https://codereview.chromium.org/1131853006/diff/20001/javatests/org/chromium/...
javatests/org/chromium/distiller/StringUtilTest.java:27:
assertTrue(StringUtil.countWords("ファイナルファンタジー") < 11);
On 2015/05/15 01:45:07, kuan wrote:
> this is a katakana sentence, how about also adding one for hiragana since u
> mentioned that in the code? u only tested a single hiragana character @33.
> 
> what about kanji, and simplified chinese?

All added.

[cjhopman, 2015-05-20 02:31:43]

2% slower seems like a large performance hit. Is that 2% total time or just 2%
in document construction time?

[wychen, 2015-05-20 07:14:58]

On 2015/05/20 02:31:43, cjhopman wrote:
> 2% slower seems like a large performance hit. Is that 2% total time or just 2%
> in document construction time?

Unfortunately it's the total time.
I'm thinking about adding a fast path for those pages without CJK characters.

[wychen, 2015-05-21 01:32:19]

Patchset #4 (id:60001) has been deleted

[wychen, 2015-05-21 01:34:21]

The speed regression should be <1% now. It's fairly difficult to measure though,
since performance measurement is inherently noisy.

[wychen, 2015-05-21 08:13:18]

On 2015/05/21 01:34:21, wychen wrote:
> The speed regression should be <1% now. It's fairly difficult to measure
though,
> since performance measurement is inherently noisy.

Performance measurement for the optimized version in patch set 4:

Before: 123.974+-0.160ms
After: 124.747+-0.143ms

Difference: 0.773+-0.214ms
Percentage: 0.62+-0.17% slower than before

[kuan, 2015-05-21 17:46:15]

i'll leave the lgtm to chris.

https://codereview.chromium.org/1131853006/diff/20001/javatests/org/chromium/...
File javatests/org/chromium/distiller/StringUtilTest.java (right):

https://codereview.chromium.org/1131853006/diff/20001/javatests/org/chromium/...
javatests/org/chromium/distiller/StringUtilTest.java:27:
assertTrue(StringUtil.countWords("ファイナルファンタジー") < 11);
On 2015/05/18 18:49:20, wychen wrote:
> On 2015/05/15 01:45:07, kuan wrote:
> > this is a katakana sentence, how about also adding one for hiragana since u
> > mentioned that in the code? u only tested a single hiragana character @33.
> > 
> > what about kanji, and simplified chinese?
> 
> All added.

thanks!

https://codereview.chromium.org/1131853006/diff/80001/javatests/org/chromium/...
File javatests/org/chromium/distiller/StringUtilTest.java (right):

https://codereview.chromium.org/1131853006/diff/80001/javatests/org/chromium/...
javatests/org/chromium/distiller/StringUtilTest.java:26:
assertTrue(StringUtil.countWords("いってらっしゃい") > 1); // Hirakana
nit: s/Hirakana/Hiragana/

[cjhopman, 2015-05-21 19:32:02]

https://codereview.chromium.org/1131853006/diff/80001/java/org/chromium/disti...
File java/org/chromium/distiller/DomDistiller.java (right):

https://codereview.chromium.org/1131853006/diff/80001/java/org/chromium/disti...
java/org/chromium/distiller/DomDistiller.java:27:
StringUtil.selectCountWordsFunc(Document.get().getBody().getInnerText());
This should use textcontent not innertext

https://codereview.chromium.org/1131853006/diff/80001/java/org/chromium/disti...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/80001/java/org/chromium/disti...
java/org/chromium/distiller/StringUtil.java:54: $wnd._countWords = function(s) {
Don't make changes to $wnd

[wychen, 2015-05-21 23:07:03]

https://codereview.chromium.org/1131853006/diff/80001/java/org/chromium/disti...
File java/org/chromium/distiller/DomDistiller.java (right):

https://codereview.chromium.org/1131853006/diff/80001/java/org/chromium/disti...
java/org/chromium/distiller/DomDistiller.java:27:
StringUtil.selectCountWordsFunc(Document.get().getBody().getInnerText());
On 2015/05/21 19:32:02, cjhopman wrote:
> This should use textcontent not innertext

Done.

https://codereview.chromium.org/1131853006/diff/80001/java/org/chromium/disti...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/80001/java/org/chromium/disti...
java/org/chromium/distiller/StringUtil.java:54: $wnd._countWords = function(s) {
On 2015/05/21 19:32:02, cjhopman wrote:
> Don't make changes to $wnd

Done.

https://codereview.chromium.org/1131853006/diff/80001/javatests/org/chromium/...
File javatests/org/chromium/distiller/StringUtilTest.java (right):

https://codereview.chromium.org/1131853006/diff/80001/javatests/org/chromium/...
javatests/org/chromium/distiller/StringUtilTest.java:26:
assertTrue(StringUtil.countWords("いってらっしゃい") > 1); // Hirakana
On 2015/05/21 17:46:15, kuan wrote:
> nit: s/Hirakana/Hiragana/

Done. Pardon my inability to spell.

[cjhopman, 2015-05-22 00:11:59]

https://codereview.chromium.org/1131853006/diff/100001/java/org/chromium/dist...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/100001/java/org/chromium/dist...
java/org/chromium/distiller/StringUtil.java:54: $_countWords = function(s) {
This is still going to modify the global window object, won't it? You just need
to set this as a static field in this class.

[cjhopman, 2015-05-22 00:13:30]

https://codereview.chromium.org/1131853006/diff/100001/java/org/chromium/dist...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/100001/java/org/chromium/dist...
java/org/chromium/distiller/StringUtil.java:54: $_countWords = function(s) {
On 2015/05/22 00:11:58, cjhopman wrote:
> This is still going to modify the global window object, won't it? You just
need
> to set this as a static field in this class.

Oh, I see what you did. Yeah, we shouldn't add globals to the linker, you should
still just have it as a static in this class.

[wychen, 2015-05-22 22:00:47]

On 2015/05/22 00:13:30, cjhopman wrote:
>
https://codereview.chromium.org/1131853006/diff/100001/java/org/chromium/dist...
> File java/org/chromium/distiller/StringUtil.java (right):
> 
>
https://codereview.chromium.org/1131853006/diff/100001/java/org/chromium/dist...
> java/org/chromium/distiller/StringUtil.java:54: $_countWords = function(s) {
> On 2015/05/22 00:11:58, cjhopman wrote:
> > This is still going to modify the global window object, won't it? You just
> need
> > to set this as a static field in this class.
> 
> Oh, I see what you did. Yeah, we shouldn't add globals to the linker, you
should
> still just have it as a static in this class.

I tried to use a static function or member in this class before using $wnd.
I cannot assign JS function to a Java static function. GWT would forbid such
assignments in JSNI.
If static member is used instead, I can trick GWT to accept assignment, even
though the type is not correct. However, it cannot be called.

Could you shed some lights on how to use static in this scenario? Thanks!

[wychen, 2015-05-23 00:32:27]

https://codereview.chromium.org/1131853006/diff/100001/java/org/chromium/dist...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/100001/java/org/chromium/dist...
java/org/chromium/distiller/StringUtil.java:54: $_countWords = function(s) {
On 2015/05/22 00:13:29, cjhopman wrote:
> On 2015/05/22 00:11:58, cjhopman wrote:
> > This is still going to modify the global window object, won't it? You just
> need
> > to set this as a static field in this class.
> 
> Oh, I see what you did. Yeah, we shouldn't add globals to the linker, you
should
> still just have it as a static in this class.

Found a workaround. PTAL.

[cjhopman, 2015-05-26 21:53:06]

https://codereview.chromium.org/1131853006/diff/120001/java/org/chromium/dist...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/120001/java/org/chromium/dist...
java/org/chromium/distiller/StringUtil.java:54: public static int _countWords;
Can't this be a JavaScriptObject? Those can store javascript functions just
fine.

Alternatively, we could do this the real Java way:

Have an interface for this, and have three implementations of it that each have
a different jsni implementation of the one function. Then just initialize a
member of that interface.

[wychen, 2015-05-29 07:44:09]

https://codereview.chromium.org/1131853006/diff/120001/java/org/chromium/dist...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/120001/java/org/chromium/dist...
java/org/chromium/distiller/StringUtil.java:54: public static int _countWords;
On 2015/05/26 21:53:06, cjhopman wrote:
> Can't this be a JavaScriptObject? Those can store javascript functions just
> fine.
> 
> Alternatively, we could do this the real Java way:
> 
> Have an interface for this, and have three implementations of it that each
have
> a different jsni implementation of the one function. Then just initialize a
> member of that interface.

Done the real Java way.

[cjhopman, 2015-05-29 19:38:57]

https://codereview.chromium.org/1131853006/diff/140001/java/org/chromium/dist...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/140001/java/org/chromium/dist...
java/org/chromium/distiller/StringUtil.java:53: private static interface
CountWords {
nit: probably should be s/CountWords/WordCounter

https://codereview.chromium.org/1131853006/diff/140001/java/org/chromium/dist...
java/org/chromium/distiller/StringUtil.java:100: static CountWords _countWords =
new FullWordCounting();
s/_countWords/sCountWords

in fact, I'd probably call this sWordCounter

https://codereview.chromium.org/1131853006/diff/140001/javatests/org/chromium...
File javatests/org/chromium/distiller/StringUtilTest.java (right):

https://codereview.chromium.org/1131853006/diff/140001/javatests/org/chromium...
javatests/org/chromium/distiller/StringUtilTest.java:57:
StringUtil.selectCountWordsFunc("");
This should probably use the inner classes directly rather than relying on the
implementation of selectCountWordsFunc

Though, we should probably have a test of selectCountWordsFunc. I think it'd be
better to sort of expose the selection itself rather than determining what was
selected based on how it counts different words, though.

[wychen, 2015-05-31 09:04:03]

https://codereview.chromium.org/1131853006/diff/140001/java/org/chromium/dist...
File java/org/chromium/distiller/StringUtil.java (right):

https://codereview.chromium.org/1131853006/diff/140001/java/org/chromium/dist...
java/org/chromium/distiller/StringUtil.java:53: private static interface
CountWords {
On 2015/05/29 19:38:57, cjhopman wrote:
> nit: probably should be s/CountWords/WordCounter

Done.

https://codereview.chromium.org/1131853006/diff/140001/java/org/chromium/dist...
java/org/chromium/distiller/StringUtil.java:100: static CountWords _countWords =
new FullWordCounting();
On 2015/05/29 19:38:57, cjhopman wrote:
> s/_countWords/sCountWords
> 
> in fact, I'd probably call this sWordCounter

Done.

https://codereview.chromium.org/1131853006/diff/140001/javatests/org/chromium...
File javatests/org/chromium/distiller/StringUtilTest.java (right):

https://codereview.chromium.org/1131853006/diff/140001/javatests/org/chromium...
javatests/org/chromium/distiller/StringUtilTest.java:57:
StringUtil.selectCountWordsFunc("");
On 2015/05/29 19:38:57, cjhopman wrote:
> This should probably use the inner classes directly rather than relying on the
> implementation of selectCountWordsFunc
> 
> Though, we should probably have a test of selectCountWordsFunc. I think it'd
be
> better to sort of expose the selection itself rather than determining what was
> selected based on how it counts different words, though.

Done.

[cjhopman, 2015-06-04 20:29:44]

lgtm

[wychen, 2015-06-05 21:01:04]

Committed patchset #8 (id:160001) manually as
7a1099255504d55d2fc2348c2d208d7679aa8195 (presubmit successful).