Closed
Bug 1239869
Opened 8 years ago
Closed 8 years ago
Remove rekeyIfMoved
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox46 | --- | affected |
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file, 1 obsolete file)
3.16 KB,
patch
|
Details | Diff | Splinter Review |
Users of these are not in the middle of an iteration, so we can replace by add/remove. While this can theoretically OOM, the other path could theoretically try to use resizeWithoutRekeying, so I think this is probably the better bet.
Attachment #8708114 -
Flags: review?(sphink)
Assignee | ||
Comment 1•8 years ago
|
||
Comment on attachment 8708114 [details] [diff] [review] remove_rekeyIfMoved-v0.diff Derp: qref failure. One sec.
Attachment #8708114 -
Attachment is obsolete: true
Attachment #8708114 -
Flags: review?(sphink)
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=008d7bfd14df
Assignee | ||
Comment 3•8 years ago
|
||
This looks more likely.
Attachment #8708123 -
Flags: review?(sphink)
Comment 4•8 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #0) > While this can theoretically OOM, the other path could > theoretically try to use resizeWithoutRekeying, so I think this is probably > the better bet. It's great to see more rekeying being removed from the hash table API. I couldn't find resizeWithoutRekeying though - do you mean rehashTableInPlace? If, so I don't see the problem with that.
Comment 5•8 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #4) > (In reply to Terrence Cole [:terrence] from comment #0) > > While this can theoretically OOM, the other path could > > theoretically try to use resizeWithoutRekeying, so I think this is probably > > the better bet. > > It's great to see more rekeying being removed from the hash table API. I > couldn't find resizeWithoutRekeying though - do you mean rehashTableInPlace? > If, so I don't see the problem with that. Yeah, that's what I'm seeing too. I think we should discuss during our call today. I don't want to agree lightly to adding unhandlable ooms.
Assignee | ||
Comment 6•8 years ago
|
||
I'm still not entirely comfortable with the current implementation of in-place rekeying, but given that we've had it deployed for years, I guess any problems would have shown up by now. As such, and given that we need to keep the Enum rekey functions for the initial shape, group, and xcomp tables anyway, let's just drop this patch. I'll make the rekey interfaces in HashTable protected, then we should be able to expose them explicitly in GCRekeyableHashTable. We'll have to make the uid table a GCRekeyableHashTable, but that should be easy enough. At that point, I think we'll have just the 4 tables that use rekeying and the damage will be limited enough for us to move on with sweeping generalizations.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Assignee | ||
Updated•8 years ago
|
Attachment #8708123 -
Flags: review?(sphink)
You need to log in
before you can comment on or make changes to this bug.
Description
•