Closed Bug 1462261 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(2 files)

      No description provided.
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.
Attachment #8976433 - Flags: review?(jdemooij)
Comment on attachment 8976433 [details] [diff] [review]
Don't use AlignedStorage2 to implement HashTableEntry

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

WFM.
Attachment #8976433 - Flags: review?(jdemooij) → review+
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?
Attachment #8976434 - Flags: review?(jdemooij) → review+
Priority: -- → P3
Pushed by jwalden@mit.edu:
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
https://hg.mozilla.org/mozilla-central/rev/233871244d80
https://hg.mozilla.org/mozilla-central/rev/353d2ee29591
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: