Closed Bug 1205132 Opened 9 years ago Closed 4 years ago

Remove AutoKeepAtoms and use proper rooting instead of suppression

Categories

(Core :: JavaScript: GC, defect)

Product:
Core
Component:
JavaScript: GC
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
82 Branch
Tracking Flags:
Tracking Status
firefox43 --- wontfix
firefox82 --- fixed

People

(Reporter: terrence, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1205132 - Remove unused AutoKeepAtoms r?tcampbell
(deleted), text/x-phabricator-request
Details
It is a hack to avoid having to root the strings in the parse nodes in the frontend. We cannot just switch them to Rooted because the nodes are arena allocated. This is an identical problem to the arena allocated IonBuilder nodes. We should generalize whatever the jit is using and use it in the frontend too.
For what it's worth, in asmjs/AsmJSValidate.cpp, ModuleValidator has a rooting note that it's fine that it contain raw pointers to PropertyNames because there's a live AutoKeepAtoms on the stack (ModuleCompiler makes the same assumption).
We should be able to make these into PersistentRooted<PropertyName*> at very low cost.
From a discussion with shu a while ago: It would be too expensive to trace the parse tree so instead the parser should maintain a set of atoms that it has used and trace that. When an atom is put into the set it should be returned cast to a different type to signify that it is rooted so that we can ensure that the parser doesn't use atoms without first putting them into the set.
This is looking less important now that bug 1213977 has landed.
Depends on: 1592105

This can be solved once frontend uses ParserAtoms in most places.
there will still be some usage of JSAtom, but they can be explicitly rooted.

(Moving this off the MVP since we will use a pref to begin with. Bug 1611437 will track the things we delete once stencil is always-on)

Blocks: 1611437
No longer blocks: stencil-mvp
Depends on: 1662374

AutoKeepAtoms is now unused so I guess we can remove it.

Assignee: nobody → jcoppeard

With the latest parser changes this class is now unnecessary. Remove the count of live AutoKeepAtoms instances on the zone and anything that used it.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/43840312ff00 Remove unused AutoKeepAtoms r=tcampbell

https://hg.mozilla.org/mozilla-central/rev/43840312ff00

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox82: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: