Closed Bug 1460416 Opened 3 years ago Closed 3 years ago

Remove unused static atoms

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

It looks like we currently have close to 300 unused static atoms, which is not great for memory usage/startup time. We should probably have static analysis that prevents us from accumulating static analysis (possibly using the symbol indexing we already do for Searchfox), but for now I just used a pretty gnarly grep and mangle script to figure out which ones are currently used.
I assume these unused atoms are "textually not present in the source code", not "never referenced during a browser run of $LENGTH".  Is that correct?

Do we believe that it's possible some of these atoms might be referenced rather heavily during certain browser usecases and are therefore worthwhile to keep around?
Priority: -- → P3
(In reply to Nathan Froyd [:froydnj] from comment #1)
> I assume these unused atoms are "textually not present in the source code",
> not "never referenced during a browser run of $LENGTH".  Is that correct?

Correct. Textually not referenced.

> Do we believe that it's possible some of these atoms might be referenced
> rather heavily during certain browser usecases and are therefore worthwhile
> to keep around?

I think that if we're going to keep around static atoms that aren't referenced by symbol name, for performance reasons, we should be smart about how we do it. I.e., we should do something like count how often a non-static atom is re-created during a PGO run, and keep the list updated as that changes.

But keeping a mixed bag of static symbols around on the basis that they might be used a bunch, without really tracking which ones actually are, seems like a bad idea.
Comment on attachment 8974529 [details]
Bug 1460416: Remove unused static atoms.

https://reviewboard.mozilla.org/r/242866/#review248714

These all look pretty reasonable to get rid of.  Thanks!
Attachment #8974529 - Flags: review?(nfroyd) → review+
Just a heads up, there's bug 1441292 for making *more* atoms static. Can you verify that these atoms don't end up as dynamic atoms in some sort of standard browsing session?
Flags: needinfo?(kmaglione+bmo)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #5)
> Just a heads up, there's bug 1441292 for making *more* atoms static. Can you
> verify that these atoms don't end up as dynamic atoms in some sort of
> standard browsing session?

Some of them might. A large number of them are definitely not referenced at all.

But I maintain my assertion that if we want to maintain a set of static atoms that aren't referenced by symbol name, we should be systematic about it, and not just keep an arbitrary mixed bag of them mixed in with the rest of the static atoms that are referenced by symbol name.
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/mozilla-central/rev/e40e77f84806
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.