Closed Bug 1239869 Opened 8 years ago Closed 8 years ago

Remove rekeyIfMoved

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox46 --- affected

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch remove_rekeyIfMoved-v0.diff (obsolete) — 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)
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)
This looks more likely.
Attachment #8708123 - Flags: review?(sphink)
(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.
(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.
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
Attachment #8708123 - Flags: review?(sphink)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: