Closed Bug 1149135 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- unaffected
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

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)
Blocks: 1148383
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+
(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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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+
You need to log in before you can comment on or make changes to this bug.