1462261 - Don't do PodZero/memset on HashTableEntry which is non-trivial

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Don't use AlignedStorage2 to implement HashTableEntry

AlignedStorage2 is deprecated, and it gives a false sense of security really.  Better to use raw aligned sized storage.

This may not strictly be *necessary* to fix this bug, but it was somewhat in the way of dealing with the warning, and this seemed a sensible time to do it.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Abandon the idea of HashTableEntry being "POD", make its constructor explicitly initialize HashTableEntry::keyHash, give it a destructor that destroys the stored T if the entry is live, and call both constructor and destructor in the necessary places

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8976433 [details] [diff] [review]
Don't use AlignedStorage2 to implement HashTableEntry

Review of attachment 8976433 [details] [diff] [review]:
-----------------------------------------------------------------

WFM.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8976434 [details] [diff] [review]
Abandon the idea of HashTableEntry being "POD", make its constructor explicitly initialize HashTableEntry::keyHash, give it a destructor that destroys the stored T if the entry is live, and call both constructor and destructor in the necessary places

Review of attachment 8976434 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/HashTable.h
@@ +834,2 @@
>  
>      void destroyIfLive() {

I think ~HashTableEntry is the only caller now (?), so maybe just inline destroyIfLive into that?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/233871244d80
Don't use AlignedStorage2 to implement HashTableEntry.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/353d2ee29591
Abandon the idea of HashTableEntry being "POD", make its constructor explicitly initialize HashTableEntry::keyHash, give it a destructor that destroys the stored T if the entry is live, and call both constructor and destructor in the necessary places.  r=jandem

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/233871244d80
https://hg.mozilla.org/mozilla-central/rev/353d2ee29591

Comment 7

[Ionuț Goldan [:igoldan]]

Significant perf wins thanks to this bug and bug 1462540!

== Change summary for alert #13332 (as of Fri, 18 May 2018 17:18:39 GMT) ==

Improvements:

  4%  JS windows7-32 pgo stylo     81,715,361.33 -> 78,556,267.33
  4%  JS windows7-32 opt stylo     81,634,055.34 -> 78,678,051.68

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13332