1460416 - Remove unused static atoms

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P3)

Description

[Kris Maglione [:kmag]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

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?

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1460416: Remove unused static atoms.

These were found using some ugly text searches, so it's possible some unused
atoms remain. In the future, we should enforce removing unused atoms using
static analysis. Or just generate the static atoms table based on string atom
names in our code.

This patch leaves unused RDF atoms in place, since those are being dealt with
in another bug.

Review commit: https://reviewboard.mozilla.org/r/242866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242866/

Comment 3

[Kris Maglione [:kmag]]

(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 4

[Nathan Froyd [:froydnj]]

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!

Comment 5

[Eric Rahm [:erahm]]

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?

Comment 6

[Kris Maglione [:kmag]]

(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.

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e40e77f848063469114960ac2f13dae5649b328f
Bug 1460416: Remove unused static atoms. r=froydnj

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e40e77f84806