Bug 46516 – Use WTF::StringHasher for hashing MappedAttributeKey

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 68753 [details]
Patch

[Adam Barth]

Comment on attachment 68753 [details]
Patch

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

Do we want to run any performance tests to make sure we're not regressing here?

> WebCore/dom/StyledElement.cpp:-456
> -    // This avoids ever returning a hash code of 0, since that is used to
> -    // signal "hash not computed yet", using a value that is likely to be
> -    // effectively the same as 0 when the low bits are masked
> -    if (hash == 0)
> -        hash = 0x80000000;

Does StringHasher respect this?

[Patrick R. Gansterer]

(In reply to comment #2)
> (From update of attachment 68753 [details])
> Do we want to run any performance tests to make sure we're not regressing here?
StringHasher has only inline methods. The only real difference is the "pendingCharacter" (http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/StringHashFunctions.h?rev=68369#L66), but a intelligent compiler should be able optimize it away.


> > WebCore/dom/StyledElement.cpp:-456
> > -    // This avoids ever returning a hash code of 0, since that is used to
> > -    // signal "hash not computed yet", using a value that is likely to be
> > -    // effectively the same as 0 when the low bits are masked
> > -    if (hash == 0)
> > -        hash = 0x80000000;
> 
> Does StringHasher respect this?
StringHasher uses 0x40000000 instead (http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/StringHashFunctions.h?rev=68369#L85), but this shoudn't make a difference, because only "not null" is relevant.

[Patrick R. Gansterer]

Created attachment 68885 [details]
Patch

Fixed copy/paste failure:
> COMPILE_ASSERT(sizeof(key.name) == 4 || sizeof(key.name) == 8, key_name_size);
> COMPILE_ASSERT(sizeof(key.value) == 4 || sizeof(key.value) == 8, key_name_size);

[Eric Seidel (no email)]

Comment on attachment 68753 [details]
Patch

Cleared Adam Barth's review+ from obsolete attachment 68753 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Patrick R. Gansterer]

(In reply to comment #2)
> (From update of attachment 68753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68753&action=review
> 
> Do we want to run any performance tests to make sure we're not regressing here?
I now run WebCore/benchmarks/parser/html-parser.html to check the difference:

BEFORE PATCH:
avg 4039.95  stdev 28.554290395665586
avg 4042.90  stdev 26.95904300972125
avg 4054.45  stdev 35.382870149268555
avg 4060.05  stdev 41.9147647017134

AFTER PATCH:
avg 4044.30  stdev 34.20833231831099
avg 4055.15  stdev 28.320090042229737
avg 4055.40  stdev 29.014479144041164
avg 4062.60  stdev 39.329886854655456

Is the difference within the measuring tolerance?

[Adam Barth]

> Is the difference within the measuring tolerance?

Yep.  Thanks.

[WebKit Commit Bot]

Comment on attachment 68885 [details]
Patch

Clearing flags on attachment: 68885

Committed r69449: <http://trac.webkit.org/changeset/69449>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.