Don't create HashMapEntry on stack when adding to a HashMap

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(firefox38 unaffected, firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8585480 [details] [diff] [review]
remove-hashmap-stack-entries

Currently the HashMap implementation temporarily creates a HashTableEntry object on the stack in add() and related methods containing the new key and value.  This is because the HashTable code is shared with the HashSet implementation and its add methods take a single argument for the entry to add.  

When either of the key or value has GC barriers this results in extra barriers being fired.  For example, hash maps whose value is a GC thing must use the RelocatablePtr<> wrapper, which this results in an unnecessary store buffer add and remove every time an entry is added to the map.

We can fix this by using variadic templates to allow us to pass any number of arguments to the add methods and get rid of the temporary object.
Attachment #8585480 - Flags: review?(luke)
(Assignee)

Updated

3 years ago
Blocks: 1148383

Comment 1

3 years ago
Comment on attachment 8585480 [details] [diff] [review]
remove-hashmap-stack-entries

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

Very nice!  I wonder if this also removes the default-constructible requirement for key/value types.
Attachment #8585480 - Flags: review?(luke) → review+
(Assignee)

Comment 2

3 years ago
(In reply to Luke Wagner [:luke] from comment #1)
It looks like it does for keys.  There's still a |Value()| in the second version of HashMap::add().
https://hg.mozilla.org/mozilla-central/rev/a362c3a2e15b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 5

3 years ago
Comment on attachment 8585480 [details] [diff] [review]
remove-hashmap-stack-entries

This change fixes bug 1148383 so requesting uplift to aurora since the change that caused that (bug 1143256) is present there.

Approval Request Comment
[Feature/regressing bug #]: Bug 1143256
[User impact if declined]: Crashes with possible security impact.
[Describe test coverage new/current, TreeHerder]: On central since yesterday.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8585480 - Flags: approval-mozilla-aurora?
Comment on attachment 8585480 [details] [diff] [review]
remove-hashmap-stack-entries

Approving for uplift to aurora since this fixes a crash.
Attachment #8585480 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox39: --- → affected
tracking-firefox39: --- → +
status-firefox38: --- → unaffected
tracking-firefox39: + → ---
You need to log in before you can comment on or make changes to this bug.